Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| const std::string hcm_str = | ||
| "envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager"; | ||
| if (is_quic && (filter_chain.filters().size() != 1 || !absl::StrContains(config_str, hcm_str) || | ||
| !absl::StrContains(config_str, "codec_type: HTTP3"))) { |
There was a problem hiding this comment.
StrContains() is inefficient and this doesn't prevent other codec types from being misconfigured to HTTP3.
I have some thoughts about validating hcm and codec_type here:
We can compare filter_chain.filters(0).typed_config().type_url() here with "type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager" to validate HCM is the only network filter configured.
As to codec_type, the caller of HttpConnectionManagerConfig::createCodec() -- Http::ConnectionManagerImpl-- actually knows if the connection is quic or not. Can we modify createCodec() interface to take in a bool and always create a Http3 codec if it's true? If the pass-in value isn't consistent with configured codec type, i.e. createCodec(/is_quic/true) with codec_type_ != HTTP3 or createCodec(/is_quic/false) with codec_type_ == HTTP3, create whatever makes the most sense and warn about that.
There was a problem hiding this comment.
switching to hcm type url makes sense.
I'm not worried about an expensive string compare for config load - it's infrequent and already expensive. I don't love doing StrContains because it's not exact, but I think it's better than adding a hard-dep from listener to HCM (will let them maintainer reviewer weigh in on that). I think ignoring configured codec type would not be Ok - it'd better to reject invalid config than accept, do something unexpeced and runtime-warn about it.
There was a problem hiding this comment.
I don't love doing the unpack twice, but why not unpack the HCM config into a real object, and then do the check in code? That would be a lot less hacky than doing str contains on a debug string?
Another option if we don't want to unpack twice. What about doing this check in the HCM config load itself? We could probably figure out a way to get back to the listener config and do the check there?
/wait-any
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| "type.googleapis.com/" | ||
| "envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager"; | ||
| if (is_quic && (filter_chain.filters().size() != 1 || | ||
| filter_chain.filters(0).typed_config().type_url() != hcm_str || |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| const std::string config_str = | ||
| filter_chain.filters_size() == 0 ? "" : filter_chain.filters(0).DebugString(); |
danzh2010
left a comment
danzh2010
left a comment
There was a problem hiding this comment.
LGTM! Thanks for plumbing quic thru network filter config!
…bridge-stream * upstream/main: (140 commits) quiche: remove google quic support (envoyproxy#17465) runtime: removing envoy.reloadable_features.check_ocsp_policy (envoyproxy#17524) upstream: not trying to do HTTP/3 where not configured (envoyproxy#17454) api: Remove confusing line about auto-generation (envoyproxy#17536) v2: removing bootstrap (envoyproxy#17523) connpool: Fix crash in pool removal if the cluster was already deleted (envoyproxy#17522) Enhance the comments clearly (envoyproxy#17517) mysql proxy: connection attributes parsing (envoyproxy#17209) [ci] fix false positive CVE scan from node (envoyproxy#17510) Fixing Envoy Mobile factory strings (envoyproxy#17509) http3: validating codec (envoyproxy#17452) quic: add QUIC upstream stream reset error stats (envoyproxy#17496) thrift proxy: move UpstreamRequest into its own file (envoyproxy#17498) docs: Fixed FaultDelay docs. (envoyproxy#17495) updates links to jaegertracing-plugin.tar.gz (envoyproxy#17497) http: make custom inline headers bootstrap configurable (envoyproxy#17330) deps: update yaml-cpp to latest master (envoyproxy#17489) improving tracer coverage (envoyproxy#17493) Increase buffer size of `Win32RedirectRecords` (envoyproxy#17471) ext_proc: Fix problem with buffered body mode with empty or no body (envoyproxy#17430) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>
Rejecting invalid config which only partially configures HTTP/3 Risk Level: low (validates broken config) Testing: new unit tests Docs Changes: n/a Release Notes: n/a Fixes envoyproxy#15985 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
3 participants
Rejecting invalid config which only partially configures HTTP/3
Risk Level: low (validates broken config)
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a
Fixes #15985