-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
allow additional network filters for QUIC listeners #22722
Conversation
Signed-off-by: Paul Sohn <paulsohn@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall!
Can you document here
repeated Filter filters = 3; |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks generally makes sense with a few comments.
/wait
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be good to mention, the HCM required to be the last filter for udp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
Signed-off-by: Paul Sohn <paulsohn@google.com>
Signed-off-by: Paul Sohn <paulsohn@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the update! one more small comment
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
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