Add an incomplete implementation of quic_hostname_utils_impl.#6146
Add an incomplete implementation of quic_hostname_utils_impl.#6146mattklein123 merged 6 commits intoenvoyproxy:masterfrom
Conversation
It is incomplete because GoogleUrl is not avaiable in Envoy yet. Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Bin Wu <wub@google.com>
|
/assign @mpwarres |
mpwarres
left a comment
There was a problem hiding this comment.
LGTM
/assign @alyssawilk
| host.erase(host_end, host.length() - host_end); | ||
| } | ||
|
|
||
| return host; |
There was a problem hiding this comment.
Do we have an ETA on GoogleUrl?
StringUtil::toLower + rfind / substr should do both for you
For SNI validity I wonder if doing a quick strcat http://sni/ and tossing it in http::Utility::Url would work for a validity check.
If we're getting GoogleUrl soon I buy it may not be worth the work.
…tname_utils Signed-off-by: Bin Wu <wub@google.com>
…validity check to QuicHostnameUtils::IsValidSNI. Signed-off-by: Bin Wu <wub@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
Cool - just one more nit and you're good to go
| // Convert hostname to lowercase and remove the trailing '.'. | ||
| // WARNING: mutates |hostname| in place and returns |hostname|. | ||
| // NOTE(wub): This only removes trailing dots for now. | ||
| // NOTE(wub): This only does lowercasing and removes trailing dots for now. |
There was a problem hiding this comment.
// Convert hostname to lowercase and remove the trailing '.'.
// NOTE(wub): This only does lowercasing and removes trailing dots for now.
can we remove the note?
There was a problem hiding this comment.
Rewrote the comment to reflect the current state. PTAL.
|
|
||
| // static | ||
| bool QuicHostnameUtilsImpl::IsValidSNI(QuicStringPiece sni) { | ||
| // TODO(wub): Implement it on top of GoogleUrl, once it is available. |
There was a problem hiding this comment.
Fine for now, but note for the SNI check if GoogleUrl takes a while we might be able to that string conversoin to IP fails, and strcat http://hostname using the existing Envoy Utl Utility passes and get full coverage without GoogleUrl
There was a problem hiding this comment.
Ack. Will definitely do a pass of TODO fixes after all platform impls are populated.
…tname_utils Signed-off-by: Bin Wu <wub@google.com>
Signed-off-by: Bin Wu <wub@google.com>
Description:
Add quic_hostname_utils_impl to QUICHE implementation. The implementation is incomplete because Envoy does not have GoogleUrl yet.
Risk Level: minimal: code not used yet
Testing:
bazel test test/extensions/quic_listeners/quiche/platform:quic_platform_test --test_output=all --define quiche=enabled --experimental_remap_main_repo
Docs Changes: none
Release Notes: none
[Optional Fixes #Issue]
[Optional Deprecated:]