You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficserver.apache.org by persiaAziz <gi...@git.apache.org> on 2017/01/23 21:18:56 UTC
[GitHub] trafficserver pull request #1357: TS-5022: reduce memory allocation in clien...
GitHub user persiaAziz opened a pull request:
https://github.com/apache/trafficserver/pull/1357
TS-5022: reduce memory allocation in clientcert lookup
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/persiaAziz/trafficserver TS-5022
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/1357.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1357
----
commit 016307460502724cf7a85812d0768f23f4d8cb99
Author: Persia Aziz <pe...@yahoo-inc.com>
Date: 2017-01-23T21:05:36Z
TS-5022: reduce memory allocation in clientcert lookup
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1357
@shinrich @SolidWallOfCode Is this landing for 7.1.x?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1357
FreeBSD build *successful*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1363/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the issue:
https://github.com/apache/trafficserver/pull/1357
The problem (as @persiaAziz patiently explained) is the client certificate is overridable which means it cannot be reliable precomputed when `remap.config` is parsed. In theory a plugin could load a distinct client certificate every single remap. Unfortunately this means checking every time. Persia said she'd look at having a single path instead of two pieces to simplify the code. I'll do more review once that's ready.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1357
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1428/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/1357
I was thinking to have one overridable config for the client cert containing the partial or absolute path to avoid the memory allocation in Layout, but that will break the convention of having separate configs for path and filename. It could be also confusing for the user
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1357
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/1321/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/1357
@SolidWallOfCode, @shinrich Please review.
In summary,
* proxy.config.ssl.client.cert.path has been completely removed.
* proxy.config.ssl.client.cert.filename has been renamed to proxy.config.ssl.client.cert.file.
* proxy.config.ssl.client.cert.file is overridable
* The user needs to specify the fullpath in proxy.config.ssl.client.cert.file.
* It does not check if the given path is absolute or not because if the path is not absolute then concatenating (default)path and filename is again going to end up in allocating memory on each remap.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1357
This has to go into 7.1.0 as well, since we already landed previous tries :).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1357
Linux build *successful*! See https://ci.trafficserver.apache.org/job/Github-Linux/1256/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by persiaAziz <gi...@git.apache.org>.
Github user persiaAziz commented on the issue:
https://github.com/apache/trafficserver/pull/1357
please review @SolidWallOfCode This will reduce the unnecessary allocations. This checks if the client cert filename and path under txn_cnf has the same value as the global defaults.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1357
FreeBSD build *failed*! See https://ci.trafficserver.apache.org/job/Github-FreeBSD/1427/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1357
@SolidWallOfCode Is this going to go in for 7.1.x ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #1357: TS-5022: reduce memory allocation in clien...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/1357#discussion_r97822859
--- Diff: proxy/http/HttpSM.cc ---
@@ -5050,9 +5036,26 @@ HttpSM::do_http_server_open(bool raw)
opt.set_sni_servername(host, len);
}
- ats_scoped_str clientCert(
- (Layout::relative_to(t_state.txn_conf->client_cert_filepath, t_state.txn_conf->client_cert_filename)));
- opt.set_client_certname(clientCert);
+ SSLConfig::scoped_config params;
+ ats_scoped_str clientCert;
--- End diff --
Why is this declared twice?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on the issue:
https://github.com/apache/trafficserver/pull/1357
I think I want a different approach on this. I'll talk with @persiaAziz.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by jpeach <gi...@git.apache.org>.
Github user jpeach commented on the issue:
https://github.com/apache/trafficserver/pull/1357
Whilst the configuration of SSL keys and certificates is a bit unfortunate, I think that it is important to be consistent.
Separately, you have to define the base directory for relative paths, otherwise the current working directory of whatever does the loading becomes an ABI.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the issue:
https://github.com/apache/trafficserver/pull/1357
Linux build *failed*! See https://ci.trafficserver.apache.org/job/Github-Linux/1320/ for details.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #1357: TS-5022: reduce memory allocation in clientcert l...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/1357
@SolidWallOfCode Ping? Also, seems this needs to be rebased now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---