allow additional network filters for QUIC listeners #22722
Conversation
Signed-off-by: Paul Sohn <paulsohn@google.com>
|
Plz also fix the clang-tidy CI |
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
| // For UDP connections, network filters can be created but do not process | ||
| // incoming data as network filters on TCP connections do (i.e. the onData method | ||
| // will never be called). |
There was a problem hiding this comment.
What's the point of this? I assume just for things like RBAC where decisions can be made at the point of initial connection? If so can you clarify? Also, do you want to phrase it more like network filters do not currently process incoming network data? As we might fix this in the future?
Can you also add a release note about this?
There was a problem hiding this comment.
Probably we shouldn't generalize it to all UDP-based connections, but be explicit about QUIC. Today QUIC doesn't pass the connection payload to network filter interface onData() because the connection itself handles multiplexing and HTTP parsing. QUIC may support non-http protocols in the future, but multiplexing always happens at L4. So QUIC L4 filters are very likely to have a different set of interfaces. So far we don't really need any L4 filter interfaces, that's why we have only allowed HCM to be installed. But we have a use case now where L4 filters can change the connection transport properties, like cwnd or socket options at the beginning of the connection. This still won't use any L4 interfaces other than the constructor. So we want to relax the constraint here.
There was a problem hiding this comment.
OK makes sense. Yes let's please be much more clear about what is supported and also add an integration test. Thank you.
There was a problem hiding this comment.
Thanks both. I've added a release note and tried to clarify the wording (let me know if you have further suggestions). I'll add an integration test next.
There was a problem hiding this comment.
I've added an integration test; please take a look.
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
| // For QUIC listeners, network filters can be created, but due to differences | ||
| // in the connection implementation compared to TCP, the onData() method will | ||
| // never be called. Therefore network filters for QUIC listeners should only | ||
| // expect to do work at the start of a new connection (i.e. in onNewConnection()). |
There was a problem hiding this comment.
it would be good to mention, the HCM required to be the last filter for udp.
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
| // to TCP, the onData() method will never be called. Therefore, network filters | ||
| // for QUIC listeners should only expect to do work at the start of a new connection | ||
| // (i.e. in onNewConnection()). If there are multiple network filters, HCM must be | ||
| // the last filter in the chain. |
There was a problem hiding this comment.
If there are multiple network filters this doesn't seems accurate, the HCM must be there and being the last one for QUIC, whatever there is only one filter or multiple filter
There was a problem hiding this comment.
Thanks, added another clarification.
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
Commit Message: Update comment re: QUIC network filters Additional Description: This comment is out of date since #22722. Signed-off-by: Paul Sohn <paulsohn@google.com>
Commit Message: Update comment re: QUIC network filters Additional Description: This comment is out of date since envoyproxy#22722. Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn paulsohn@google.com
Commit Message: For QUIC listeners, allow network filters to be added before HTTP Connection Manager
Additional Description: Note that onData() is still never called on QUIC.
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A