websocket: websocket codec implementation

[Amila-Rukshan]

Commit Message: websocket: websocket codec and frame inspector added

Testing: Unit tests added

Additional Description: Related issue comment => https://github.com/envoyproxy/envoy/issues/13877#issuecomment-755722888 This is as the initial step to support rate limiting for WebSocket.

Risk Level: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional API Considerations:]

[Amila-Rukshan]

Is the input required to contain only whole frames, or can it also contain partial frames.

both ways are valid inputs. input will drain only up to the complete frames (successfully decoded frames).

Relatedly, can a particular slice contain a partial frame? Like is it possible for the length field to be split across two different slices? I don't think I saw tests for this behaviour.

Yes these are possibilities, tests have been added now.

[RyanTheOptimist]

/assign @ggreenway

[RyanTheOptimist]

@ggreenway for a non-Google review

[ggreenway]

I'm going to be out the rest of this week. @wbpcode can you do a non-google review on this?

[RyanTheOptimist]

@wbpcode friendly ping

[wbpcode]

Hi, thanks for this great contribution. I have taken a quick look and it's great.

But I am not expert of websocket and this is a big PR. So I will review it again more carefully this week. Thanks again.

[soulxu]

@Amila-Rukshan thanks for the updating to add an option not to buffer the payload!

But my second comment seems ignored https://github.com/envoyproxy/envoy/pull/22542#discussion_r950022717 Since the length of payload can be huge, should we enable option for the caller limits the length of payload to buffer. I still think it isn't safe to buffer the payload unlimited. but you could let me know your opinion on this.

Other things coming to my mind after thinking of the use case of this WebSocket codec, its can be:

• the filter only care about the metadata of WebSocket frame, that is can be done by the decode_payload option which you just add in the last update
• The filter wants the payload, but won't stop the filter iteration, the WebSocket codec will buffer the payload for it. That is available from your implement also.
• the filter wants the payload but will stop the filter iteration until it got the expected data.

For the last case, the HTTP filter actually can stop the filter iteration and ask the HCM(filter manager) to buffer the data, the HTTP filter just need to return those status: https://github.com/envoyproxy/envoy/blob/068861364d84ba86f8b6bdc57c637afb10eb6692/envoy/http/filter.h#L118-L138

Then the filter can access the buffered decode data through: https://github.com/envoyproxy/envoy/blob/068861364d84ba86f8b6bdc57c637afb10eb6692/envoy/http/filter.h#L365-L369

So I'm thinking about this case, whether we can ask the WebSocket codec to reference the offset in the existing buffered decode data than making a copy?

[soulxu]

@Amila-Rukshan thanks for the updating to add an option not to buffer the payload!

But my second comment seems ignored #22542 (comment) Since the length of payload can be huge, should we enable option for the caller limits the length of payload to buffer. I still think it isn't safe to buffer the payload unlimited. but you could let me know your opinion on this.

Other things coming to my mind after thinking of the use case of this WebSocket codec, its can be:

• the filter only care about the metadata of WebSocket frame, that is can be done by the decode_payload option which you just add in the last update
• The filter wants the payload, but won't stop the filter iteration, the WebSocket codec will buffer the payload for it. That is available from your implement also.
• the filter wants the payload but will stop the filter iteration until it got the expected data.

For the last case, the HTTP filter actually can stop the filter iteration and ask the HCM(filter manager) to buffer the data, the HTTP filter just need to return those status:

https://github.com/envoyproxy/envoy/blob/068861364d84ba86f8b6bdc57c637afb10eb6692/envoy/http/filter.h#L118-L138

Then the filter can access the buffered decode data through:

https://github.com/envoyproxy/envoy/blob/068861364d84ba86f8b6bdc57c637afb10eb6692/envoy/http/filter.h#L365-L369

So I'm thinking about this case, whether we can ask the WebSocket codec to reference the office in the existing buffered decode data than making a copy?

ping maintainers @RyanTheOptimist @ggreenway @wbpcode, ensure I'm dragging the PR in the wrong direction :)

[wbpcode]

@soulxu Thanks. Your reviews are very valuable.

Since the length of payload can be huge, should we enable option for the caller limits the length of payload to buffer. I still think it isn't safe to buffer the payload unlimited. but you could let me know your opinion on this.

I agree with you. A more specific limit configuration would be better.

So I'm thinking about this case, whether we can ask the WebSocket codec to reference the office in the existing buffered decode data than making a copy?

IMO, it's OK to create a copy if the specific decode_payload is set to true. It will ensure the payload can be accessed safely after the buffer is drained. We can do the memory optimization in the future if it's necessary indeed.

[soulxu]

@soulxu Thanks. Your reviews are very valuable.

Since the length of payload can be huge, should we enable option for the caller limits the length of payload to buffer. I still think it isn't safe to buffer the payload unlimited. but you could let me know your opinion on this.

I agree with you. A more specific limit configuration would be better.

Thanks!

So I'm thinking about this case, whether we can ask the WebSocket codec to reference the office in the existing buffered decode data than making a copy?

IMO, it's OK to create a copy if the specific decode_payload is set to true. It will ensure the payload can be accessed safely after the buffer is drained. We can do the memory optimization in the future if it's necessary indeed.

Yea, agree that can be implemented in the next step. Also, the HCM's existing decoded data buffer also provides the buffer limit protection. And that is configurable. seems a good choice also. Anyway, totally fine for next step.

[soulxu]

LGTM, thanks!

@wbpcode @ggreenway for maintainer review