Listener FilterChainMatch using connection metadata#19950
Listener FilterChainMatch using connection metadata#19950ssripadham wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
|
Hi @ssripadham, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
There was a problem hiding this comment.
Why do we have a new notion of connection metadata? Doesn't the StreamInfo associated with a connection, providing both dynamic metadata and FilterState suffice?
/wait
There was a problem hiding this comment.
@htuch - First of all, thanks @htuch for reviewing the code. I am not familiar with StreamInfo. Are you referring to https://www.envoyproxy.io/docs/envoy/v1.21.0/configuration/http/http_filters/lua_filter.html?highlight=streaminfo#config-http-filters-lua-stream-info-wrapper ? In any case, we will still need the changes in FilterChainMatch to match based in the dynamic metadata correct?
There was a problem hiding this comment.
FilterChain matching is bound to the listener (socket) and is based on Network::ConnectionSocket object.
In this PR, we are attaching the metadata to Network::ConnectionSocket.
During filter chain match, this is referenced as below:
FilterChainManagerImpl::findFilterChain(const Network::ConnectionSocket& socket) const {
const auto& connection_metadata = absl::AsciiStrToLower(socket.connectionInfoProvider().connectionMetadata());
...
However, streamInfo is on Connection itself and not on the Socket and is referenced as below connection().streamInfo().setDynamicMetadata()
How do you propose we use streamInfo to do filterChain match?
There was a problem hiding this comment.
The caller should have access to this, e..g. in ActiveStreamListenerBase::newConnection. @lambdai does this work?
There was a problem hiding this comment.
Right, the change in this PR kicks in filterChainManager().findFilterChain(*socket) in the code below. So it will still require the socket to hold the metadata and the findFilterChain to select the filter chain based on the metadata.
void ActiveStreamListenerBase::newConnection(Network::ConnectionSocketPtr&& socket,
std::unique_ptr<StreamInfo::StreamInfo> stream_info) {
// Find matching filter chain.
const auto filter_chain = config_->filterChainManager().findFilterChain(*socket);
...
}
There was a problem hiding this comment.
Unified matcher has been merged #20110. We still need a metadata matcher but that would be an extension now.
There was a problem hiding this comment.
@kyessenov - This is great to hear. Could you please share the design for Unified matcher and how to integrate metadata matcher to unified filter?
There was a problem hiding this comment.
There was a problem hiding this comment.
Hey @kyessenov - we are unable to use the unified matcher as it doesn't support custom matcher we are building for dynamic metadata matching #23503
There was a problem hiding this comment.
@kyessenov - Is there a way to support dynamic metadata matching in addition to static string matching in Unified matcher?
Co-authored-by: Piumi Abeynayaka <61301888+PiumiAbeynayaka@users.noreply.github.com> Signed-off-by: Sudharsan Sripadham <4796246+ssripadham@users.noreply.github.com>
Co-authored-by: Piumi Abeynayaka <61301888+PiumiAbeynayaka@users.noreply.github.com> Signed-off-by: Sudharsan Sripadham <4796246+ssripadham@users.noreply.github.com>
Co-authored-by: Piumi Abeynayaka<61301888+PiumiAbeynayaka@users.noreply.github.com> Signed-off-by: Sudharsan Sripadham <4796246+ssripadham@users.noreply.github.com>
|
/wait |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Commit Message: FilterChainMatch using metadata on the connection.
Co-authored-by: Piumi Abeynayaka
Additional Description:
config.listener.v3.FilterChainMatch¶
specifies the match criteria for selecting a specific filter chain for a listener.
The following order applies:
This pull request adds connection metadata to this list before the Destination port is matched.
This is somewhat related to #18856
Risk Level: Low
Testing: Unit Tests have been added to test different scenarios
Docs Changes: Documentation here (https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto.html?highlight=filterchainmatch) should be enhanced to hilight the new order.
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]