containerd
client, server: implement configurable wire message size limits.
This PR implements configurable limits for the maximum accepted message size of the wire protocol. The default limits can be overridden using the new WithClientWireMessageLimit() option for clients and WithServerWireMessageLimit() option for servers.
In particular this PR includes commits to
- implement the new options for setting client and server side message size limits
- add new unit tests to exercise these options
- update the protocol description to reflect the increased max. message size
@dmcgowan These options accept a maximum limit of 2^31-1 and a minimum limit of 4K. Therefore this PR effectively changes the protocol (as reflected in the updated protocol description) to use a reserved most significant bit in the frame data length field instead of a reserved most significant byte. Is this fine ? Or should we leave a few more bits reserved for future use, for instance by limiting the max. wire message size to 256M-1 leaving 4 bits unused ?
Updates:
- max. message size reverted back to 16M
- reverted defaults to no sender-side length check
- added propagation of receiver-caught length errors back to sender
update the protocol description to reflect the increased max. message size
I wonder if this is necessary. The protocol allows for 16 MB but then we hardcode in the limit of 4 MB. I wonder if we can limit this change to make it configurable but keep the protocol max at 16 MB. We also don't seem to properly implement what the current language is, if the first byte is non-zero we should immediately fail with a protocol error rather than attempting to discard more than what the protocol allows.
While I'm sure there are use cases where more than 16 MB could be used, I'm not sure about just making this almost boundless on an object that is expected to be held and parsed in memory. Its better for the rpcs on top to implement their own streaming or framing.
Related to server/client agreeing. It is possible that we would want a client that maintains a 4 MB send since it may not know whether the server supports more, although allowing a larger response. Is that currently possible here? Could also be useful to allow getting the behavior it is now where the client does not even enforce the limit but just allows the server to fail. In that case it would be good to preserve the "max" in the error from the server.
update the protocol description to reflect the increased max. message size
I wonder if this is necessary. The protocol allows for 16 MB but then we hardcode in the limit of 4 MB. I wonder if we can limit this change to make it configurable but keep the protocol max at 16 MB. We also don't seem to properly implement what the current language is, if the first byte is non-zero we should immediately fail with a protocol error rather than attempting to discard more than what the protocol allows.
While I'm sure there are use cases where more than 16 MB could be used, I'm not sure about just making this almost boundless on an object that is expected to be held and parsed in memory. Its better for the rpcs on top to implement their own streaming or framing.
Good point. I can set the max. allowed message size to 16MB.
Related to server/client agreeing. It is possible that we would want a client that maintains a 4 MB send since it may not know whether the server supports more, although allowing a larger response. Is that currently possible here? Could also be useful to allow getting the behavior it is now where the client does not even enforce the limit but just allows the server to fail. In that case it would be good to preserve the "max" in the error from the server.
I can take a stab at it and see if I can rework the PR with these in mind. I think if we split the current max. allowed length to a max. allowed send and a max. allowed receive limit, plus update both the client and server side options for setting the send limit to interpret 0 as "no limit/don't check", then we could do all of the above. And we could probably also then restore the default behavior (no client or server options used) to the original/current one.
@dmcgowan Here is my first stab at addressing the review comments:
This now
- is rebased on #171 which already has one approval and probably should go in first
- keeps the current protocol-allowed max. unchanged at 16MB
What is still missing are
- allow disabling sender side length check to mimic original behavior
- propagate receiver-reported max. message if the message was rejected by the receiver
@dmcgowan Here is my first stab at addressing the review comments:
This now
- is rebased on channel: reject oversized messages on the sender side(, too). #171 which already has one approval and probably should go in first
- keeps the current protocol-allowed max. unchanged at 16MB
What is still missing are
- allow disabling sender side length check to mimic original behavior
- propagate receiver-reported max. message if the message was rejected by the receiver
@dmcgowan Updated to
- add propagation of receiver-reported message size errors back to client, and
- revert default behavior (with no limits set) to disable sender-side size checks
I'm not sure if disabled sender-side size check is still the default behavior we want, or if we'd rather just want a programmatic way to explicitly restore the old behavior. If it is the latter, I think we could do that by providing extra ReceiveOnlySizeCheck(size int) {ClientOpts,ServerOpt} and small changes to clampWireMessageLimit() and channel.maxMsgLimit(). Just let me know which is your preference and I can try making these changes if necessary.