search

envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.37k stars 4.71k forks source link

How to deal with maxReadBytes appropriately when adding listener filters after tls inspector? #35079

Open zhaoyangli311 opened 3 weeks ago

zhaoyangli311 commented 3 weeks ago

Title: How to deal with maxReadBytes appropriately when adding listener filters after tls inspector?

Description:

I have a use case that several listener filters are added after tls inspector, and these listener filters don't need to peek any data, but only need to return Network::FilterStatus::StopIteration in Filter::onAccept, wait for asynchronous response from some other services, and resume when response is ready. https://github.com/envoyproxy/envoy/blob/caaafa1ca69b10ed0c56739f64885259e7bae65a/source/common/listener_manager/active_tcp_socket.cc#L121-L133 The maxReadBytes of tls inspector is TLS_MAX_CLIENT_HELLO by default, and can be adjusted dynamically by setting initial_read_buffer_size in proto config. In order for these listener filters to wait for response by returning StopIteration, maxReadBytes of them must be set to TLS_MAX_CLIENT_HELLO, or even need to be synced dynamically with result of requested_read_bytes_ of tls inspector when initial_read_buffer_size is set, so that it doesn't break below assertion. https://github.com/envoyproxy/envoy/blob/caaafa1ca69b10ed0c56739f64885259e7bae65a/source/common/listener_manager/active_tcp_socket.cc#L85-L92

May I know what is the suggestion on dealing with maxReadBytes for listener filters which don't need to peek any data but need to wait for asynchronous response from other services? Can I also get some advice from you @soulxu? Since I see your discussions and contributions around ListenerFilterBufferImpl, Thanks.

soulxu commented 3 weeks ago

@zhaoyangli311 I guess I think all the filters which stop the iteration at onAccept is for inspect the data later in the beginning. But yes, in your case, you just want to stop the iteration, but not looking for any data, then the maxReadBytes suppose to be 0. To enable this case, we can remove the assertion and skip event activation when the maxReadBytes is 0.

For now, a workaround is that you have to set the maxReadBytes to a non-zero value.

soulxu commented 3 weeks ago

Or we can add one more status Network::FilterStatus::StopIterationAndWaitData for the case of filter waiting for more data. The Network::FilterStatus::StopIteration is for your case.

zhaoyangli311 commented 3 weeks ago

Option 1 is a quick fix for the issue. Option 2 solves the problem under the hood. Personally I prefer Option 2 since it decouples different scenarios just like FilterHeadersStatus, but it involves changes on api and many existing filters which are using Network::FilterStatus::StopIteration. I would like to take this enhancement, but considering timeline, I may want to separate it into 2 milestones -- first go with Option 1, and in the Meanwhile, deep dive into changes needed, and start a thread to get more agreements on this approach. What do you think?

soulxu commented 3 weeks ago

Yes, option2 is better. I would say we should go to the option2 directly. it won't take too much effort to change existing filters.

zhaoyangli311 commented 2 weeks ago

I see, let me start to work on option 2.