ssl: add support for SNI. #1984
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -86,7 +86,7 @@ ServerContextPtr ContextManagerImpl::createSslServerContext( | |
| ServerContext* ContextManagerImpl::findSslServerContext(const std::string& listener_name, | ||
| const std::string& server_name) { | ||
| std::shared_lock<std::shared_timed_mutex> lock(contexts_lock_); | ||
|
There was a problem hiding this comment. To make sure we don't mess up the reader-writer lock semantics, can this function be const? Or can we only access the member variables through a const-ref? Then the compiler will error if anything non-const is accidentally done. Or maybe put most of the logic into a static member that takes the maps by const-ref parameter. |
||
| if (map_exact_[listener_name].find(server_name) != map_exact_[listener_name].end()) { | ||
|
There was a problem hiding this comment. This is functionally correct, but it uses 5 hash table lookups when it only needs to do two. Should be: auto& listener_map = map_exact_[listener_name]; Same in other places using this map. |
||
| return map_exact_[listener_name][server_name]; | ||
| } | ||
|
|
||
|
|
@@ -97,14 +97,18 @@ ServerContext* ContextManagerImpl::findSslServerContext(const std::string& liste | |
| size_t rpos = server_name.rfind('.'); | ||
| if (rpos > pos + 1 && rpos != server_name.size() - 1) { | ||
| std::string wildcard = '*' + server_name.substr(pos); | ||
|
There was a problem hiding this comment. nit: any reason to not use the wildcard matching algorithm that we have for virtual hosts? There was a problem hiding this comment. +1 to reusing the matching code in config_impl. Can we factor that out into a utility? There was a problem hiding this comment. Which code do you mean, exactly? There was a problem hiding this comment. source/common/router/config_impl.h, RouteMatcher does similar work. See "domains" in https://www.envoyproxy.io/envoy/configuration/http_conn_man/route_config/vhost.html#config-http-conn-man-route-table-vhost. Without a strong argument against, I think it makes sense for the matching semantics to be the same between this code and that. There was a problem hiding this comment. I'd +1 as well for the reason that we might change the implementation to be smarter one day, e.g. trie-based as per @tschroed 's earlier implementation. Probably not needed right now, but just as future proofing. |
||
| if (map_wildcard_[listener_name].find(wildcard) != map_wildcard_[listener_name].end()) { | ||
| return map_wildcard_[listener_name][wildcard]; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (map_exact_[listener_name].find(EMPTY_STRING) != map_wildcard_[listener_name].end()) { | ||
| return map_exact_[listener_name][EMPTY_STRING]; | ||
| } | ||
|
|
||
| return nullptr; | ||
| } | ||
|
|
||
| size_t ContextManagerImpl::daysUntilFirstCertExpires() { | ||
|
|
||
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.
can you throw a TODO here (attributed to me if you want) about removing locking from this function. With R/W lock it's fine but would prefer to ultimately make it lockless.
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. I think we could fix that by using
std::shared_ptr, but I didn't want to block this PR on it. Also, keep in mind that whatever we're going to do now, it's most likely going to change with SDS.