You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2021/06/21 08:51:23 UTC
[GitHub] [trafficserver] maskit opened a new pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
maskit opened a new pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961
I guess some people don't like this change because there are many `dynamic_cast`s. I can add functions like `is_tls_supported()`, `is_alpn_supported()`, etc., and make the casts to `static_cast`s if we prefer it, but that would make code fragile a little. We'd need to maintain the consistency between `TLSSomethingSupport` and `is_something_supported()`on each NetVC types.
If the both this PR and #7959 are merged, a couple of log fields such as `cqssl`, `cqssv`, `cqssc` will be available on QUIC connections.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655499685
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
I'm also fine with it. One thing I don't like very much is that we'd need to have a function for each service. Is it possible to have an universal function like this?
```
T *getService<T>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655449959
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Yes, the multiple `dynamic_cast` operations are a concern. One alternative would be to put a virtual method on `UnixNetVConnection` like
```
virtual TLSBasicSupport * tls_services() { return nullptr; }
```
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Yes, but I might need to bring in a bit of meta programming support from libSWOC. You'd want something like
```
template<typename T> auto get_service_for() -> EnableForServiceTypes<T, T*> { return static_cast<T*>(this); }
```
This requires a type list backing it up, which lists all of the types for which there is a service. This compiles for those types and not for other types.
A problem is template functions/methods cannot be also `virtual`. You must choose between the cost of `dynamic_cast` or a fatter base API.
It would be possible for the mixins to provide the methods, but it would still be necessary to put a `using` in the inheriting class
```
class QUIC : public TicketService {
using TicketService::ticker_service();
};
```
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Yes, but I might need to bring in a bit of meta programming support from libSWOC. You'd want something like
```
template<typename T> auto get_service_for() -> EnableForServiceTypes<T, T*> { return static_cast<T*>(this); }
```
This requires a type list backing it up, which lists all of the types for which there is a service. This compiles for those types and not for other types.
A problem is template functions/methods cannot be also `virtual`. You must choose between the cost of `dynamic_cast` or a fatter base API.
It would be possible for the mixins to provide the methods, but it would still be necessary to put a `using` in the inheriting class
```
class QUIC : public TicketService {
using TicketService::ticket_service();
};
```
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Thinking about this more, I'm not sure why `feature_for<TLSBasicSupport>()` is much better than `tls_feature()`. It's only three methods which isn't much bloat and in the end, these feature sets are chunky and important enough that it's reasonable to promote virtual methods for them to `UnixNetVConnection`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655558423
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Yes, but I might need to bring in a bit of meta programming support from libSWOC. You'd want something like
```
template<typename T> auto get_service_for() -> EnableForServiceTypes<T, T*> { return static_cast<T*>(this); }
```
This requires a type list backing it up, which lists all of the types for which there is a service. This compiles for those types and not for other types.
A problem is template functions/methods cannot be also `virtual`. You must choose between the cost of `dynamic_cast` or a fatter base API.
It would be possible for the mixins to provide the methods, but it would still be necessary to put a `using` in the inheriting class
```
class QUIC : public TicketService {
using TicketService::ticker_service();
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655558423
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Yes, but I might need to bring in a bit of meta programming support from libSWOC. You'd want something like
```
template<typename T> auto get_service_for() -> EnableForServiceTypes<T, T*> { return static_cast<T*>(this); }
```
This requires a type list backing it up, which lists all of the types for which there is a service. This compiles for those types and not for other types.
A problem is template functions/methods cannot be also `virtual`. You must choose between the cost of `dynamic_cast` or a fatter base API.
It would be possible for the mixins to provide the methods, but it would still be necessary to put a `using` in the inheriting class
```
class QUIC : public TicketService {
using TicketService::ticket_service();
};
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655630773
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Thinking about this more, I'm not sure why `feature_for<TLSBasicSupport>()` is much better than `tls_feature()`. It's only three methods which isn't much bloat and in the end, these feature sets are chunky and important enough that it's reasonable to promote virtual methods for them to `UnixNetVConnection`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] SolidWallOfCode commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
SolidWallOfCode commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655449959
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
Yes, the multiple `dynamic_cast` operations are a concern. One alternative would be to put a virtual method on `UnixNetVConnection` like
```
virtual TLSBasicSupport * tls_services() { return nullptr; }
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 commented on pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
masaori335 commented on pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#issuecomment-866439636
IIUC, if we have more classes in the hierarchy, `dynamic_cast` gets slower. /cc @SolidWallOfCode @ywkaras
But, yes, let's measure the performance impact. FWIW, last time I checked, `dynamic_cast` in the HttpSM had 5% overhead in total.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#issuecomment-868213470
Quick performance test on a test server didn't show noticeable difference on RPS and CPU usage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on a change in pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit commented on a change in pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#discussion_r655499685
##########
File path: proxy/http/HttpSM.cc
##########
@@ -540,30 +540,35 @@ HttpSM::attach_client_session(ProxyTransaction *client_vc)
// Collect log & stats information. We've already verified that the netvc is !nullptr above,
// and netvc == ua_txn->get_netvc().
- SSLNetVConnection *ssl_vc = dynamic_cast<SSLNetVConnection *>(netvc);
is_internal = netvc->get_is_internal_request();
mptcp_state = netvc->get_mptcp_state();
client_tcp_reused = !(ua_txn->is_first_transaction());
- if (ssl_vc != nullptr) {
+ if (auto tbs = dynamic_cast<TLSBasicSupport *>(netvc)) {
Review comment:
I'm also fine with it. One thing I don't like very much is that we'd need to have a function for each service. Is it possible to have an universal function like this?
```
T *getService<T>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit merged pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] masaori335 edited a comment on pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
masaori335 edited a comment on pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#issuecomment-866439636
IIUC, if we have more classes in the hierarchy, `dynamic_cast` gets slower. /cc @SolidWallOfCode @ywkaras
But, yes, let's measure the performance impact. FWIW, last time I checked, `dynamic_cast` in the HttpSM had about 5% overhead in total.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#issuecomment-865070447
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#issuecomment-865436898
I'm going to land this as is to move forward if there is no strong objection. We'll revisit this if we see unacceptable performance decrease.
Currently only 3 features (Basic, ALPN, SessionResumption) are used here, but we have SNI and I have unfinished code for EarlyData and ClientCertVerification. And as you can see we should probably have one for Tunnel. I agree we should not do costly dynamic_cast for each if we use all of them here, but I don't think it's a great design to require having 7 getters for them and adding one for each new feature, in terms of OOP.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #7961: Don't rely on SSLNetVC when HttpSM gathers info about SSL
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #7961:
URL: https://github.com/apache/trafficserver/pull/7961#issuecomment-865070447
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org