Conversation
Coverage Report for CI Build 24791090420Warning No base build found for commit Coverage: 100.0%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
|
|
||
| ConnectionServiceOptions connection_service_options = 8; | ||
|
|
||
| repeated string enabled_channel_filters = 9; |
There was a problem hiding this comment.
What is the intended relationship between enabled_channel_filters here and the UpstreamTarget channel_filters below?
I'm not sure I understand why this field takes just the filter name while that one takes a full TypedExtensionConfig.
There was a problem hiding this comment.
This should be fixed, I hadn't actually wired up the channel_filters configuration yet and realized I was missing some config setup code here. enabled_channel_filters controls which channel filter factory instances are created for new ssh connections. These can have their own typed config, which controls the factory itself. Then, the extension configs in channel_filters are checked against the list of enabled filters and validated, and when a channel is created a read filter or write filter is created from each configured factory. The channel filters themselves have a separate config type.
There was a problem hiding this comment.
I think the naming is confusing me: if both of these end in channel_filters I would expect them to take the same kind of config.
Would it make sense to rename this one to channel_filter_factories? (or enabled_channel_filter_factories, if you want to keep the enabled bit)
| struct unused_in_this_test {}; | ||
| ChannelFilterManager(unused_in_this_test) {} |
There was a problem hiding this comment.
If this is a special test-only constructor? Is there some way we could keep this out of the production code?
There was a problem hiding this comment.
Yes this is for tests which cannot obtain a ServerFactoryContext. I don't think there is a way to #ifdef this out for tests only.
| virtual ChannelFilterPtr createReadFilter(ChannelFilterCallbacks& channel_callbacks) PURE; | ||
| virtual ChannelFilterPtr createWriteFilter(ChannelFilterCallbacks& channel_callbacks) PURE; |
There was a problem hiding this comment.
I'm not sure I understand how the distinction between "read filter" and "write filter" relates to the ChannelFilter type, as I see only the one onMessageForward() method there. Can you help me understand the intended design?
There was a problem hiding this comment.
Read filters are passed downstream->upstream forwarded messages, and Write filters are passed upstream->downstream forwarded messages. I could rename these createDownstreamFilter and createUpstreamFilter but I don't know if that would be more or less confusing.
There was a problem hiding this comment.
Could you add some comments here to help explain the terminology?
I'm still not sure I understand:
- Does this mean that each configured filter is effectively inserted twice, once on the downstream → upstream direction and once on the upstream → downstream direction?
- Do we anticipate that individual filters would need to distinguish between messages forwarded in one direction vs. the other?
- Do we anticipate wanting the ability to insert a filter only in one direction or the other?
kralicky
force-pushed
the
kralicky/channel-filters
branch
4 times, most recently
from
9c8be7c to
0000ff4
Compare
| private: | ||
| Envoy::Server::Configuration::ServerFactoryContext* context_{}; | ||
| std::unordered_map<std::string, ChannelFilterFactoryPtr> factories_; | ||
| std::unordered_map<std::string, ProtobufTypes::MessagePtr> filter_configs_; |
There was a problem hiding this comment.
Can you help me understand the lifecycle of ChannelFilterManager? The mutable state here makes me nervous.
It looks like there is one ChannelFilterManager per SshCodecFactory — does that mean the manager is shared across multiple ssh connections?
There was a problem hiding this comment.
Is this file deletion intended?
kralicky
force-pushed
the
kralicky/channel-filters
branch
from
3570f81 to
4057d7f
Compare
kralicky
force-pushed
the
kralicky/channel-filters
branch
from
4057d7f to
ebc948f
Compare
3 participants
This adds a new channel filter extension type which can be used in a similar manner as generic proxy stream filters, but scoped to individual ssh channels instead of the entire connection. Channel filters implement a method
onMessageForwardwhich is called before a channel is about to forward a message to the peer. Channel filters are also able to query a channel's internal id, type, and stats, and are able to interrupt channels using the same preempt mechanism used for graceful shutdown.