[PATCH] ConnStateData flexible transport support

2014-04-28 Thread Amos Jeffries
We are quickly approaching a time when a client connection can freely
migrate between protocols or versions of protocols. Already we have
ssl-bump which can switch a connection from HTTP to HTTPS. I am
expecting switching HTTP-HTTPS via Upgrade, and HTTP/1-HTTP/2 via
magic, Upgrade, or ALPN.

Based on ssl-bump experience with switchedToHttps() and the pain that
can be predicted when there are several permutations of such accessors
to test against I am proposing the attached patch.


* Add a transportVersion member to ConnStateData which holds the current
protocol to be used over the clientConnection socket. This variable can
be altered whenever necessary to cause an on-wire protocol change. New
connections default to the protocol signalled in the http(s)_port directive.

* ssl-bump transforms the transportVersion from whatever it was
previously (usually HTTP or HTTPS) to HTTPS.
 - Also updated ssl-bump to set the traffic type flag tunnelSslBumped on
non-intercept traffic, which can be assumed to be a CONNECT request.

* transparent and reverse-proxy URL reconstruction is updated to use the
new member instead of the http(s)_port protocol= setting. This removes
edge conditions where the URL reconstructor needs to figure out ssl-bump
existence.


Christos,
 I would like some help with two ssl-bump related things if you have the
time spare:

 1) testing the new prepareTransparentURL reconstruction works on
ssl-bumped traffic.

 2) finding switchedToHttps() usage that can be replaced.
  When ssl-bump is operating we should now always have one of these
conditions being true:
   a) ConnStateData::port-flags.tunnelSslBump
   b) ConnStateData::port-intercepted() 
ConnStateData::transportVersion.protocol==AnyP::PROTO_HTTPS

The second one is low-priority even if this patch gets added. We will
find them later anyway as the code polishing progresses.

Amos
=== modified file 'src/client_side.cc'
--- src/client_side.cc  2014-04-28 10:28:00 +
+++ src/client_side.cc  2014-04-28 10:28:59 +
@@ -2099,94 +2099,92 @@
 const bool switchedToHttps = conn-switchedToHttps();
 const bool tryHostHeader = vhost || switchedToHttps;
 if (tryHostHeader  (host = mime_get_header(req_hdr, Host)) != NULL) {
 debugs(33, 5, ACCEL VHOST REWRITE: vhost=  host   + vport=  
vport);
 char thost[256];
 if (vport  0) {
 thost[0] = '\0';
 char *t = NULL;
 if (host[strlen(host)] != ']'  (t = strrchr(host,':')) != NULL) {
 strncpy(thost, host, (t-host));
 snprintf(thost+(t-host), sizeof(thost)-(t-host), :%d, vport);
 host = thost;
 } else if (!t) {
 snprintf(thost, sizeof(thost), %s:%d,host, vport);
 host = thost;
 }
 } // else nothing to alter port-wise.
 int url_sz = strlen(url) + 32 + Config.appendDomainLen +
  strlen(host);
 http-uri = (char *)xcalloc(url_sz, 1);
-const char *protocol = switchedToHttps ?
-   https : 
AnyP::UriScheme(conn-port-transport.protocol).c_str();
-snprintf(http-uri, url_sz, %s://%s%s, protocol, host, url);
+snprintf(http-uri, url_sz, %s://%s%s, 
AnyP::UriScheme(conn-transportVersion.protocol).c_str(), host, url);
 debugs(33, 5, ACCEL VHOST REWRITE: '  http-uri  ');
 } else if (conn-port-defaultsite /*  !vhost */) {
 debugs(33, 5, ACCEL DEFAULTSITE REWRITE: defaultsite=  
conn-port-defaultsite   + vport=  vport);
 int url_sz = strlen(url) + 32 + Config.appendDomainLen +
  strlen(conn-port-defaultsite);
 http-uri = (char *)xcalloc(url_sz, 1);
 char vportStr[32];
 vportStr[0] = '\0';
 if (vport  0) {
 snprintf(vportStr, sizeof(vportStr),:%d,vport);
 }
 snprintf(http-uri, url_sz, %s://%s%s%s,
- AnyP::UriScheme(conn-port-transport.protocol).c_str(), 
conn-port-defaultsite, vportStr, url);
+ AnyP::UriScheme(conn-transportVersion.protocol).c_str(), 
conn-port-defaultsite, vportStr, url);
 debugs(33, 5, ACCEL DEFAULTSITE REWRITE: '  http-uri ');
 } else if (vport  0 /*  (!vhost || no Host:) */) {
 debugs(33, 5, ACCEL VPORT REWRITE: http_port IP + vport=  vport);
 /* Put the local socket IP address as the hostname, with whatever 
vport we found  */
 int url_sz = strlen(url) + 32 + Config.appendDomainLen;
 http-uri = (char *)xcalloc(url_sz, 1);
 http-getConn()-clientConnection-local.toHostStr(ipbuf,MAX_IPSTRLEN);
 snprintf(http-uri, url_sz, %s://%s:%d%s,
- AnyP::UriScheme(conn-port-transport.protocol).c_str(),
+ AnyP::UriScheme(conn-transportVersion.protocol).c_str(),
  ipbuf, vport, url);
 debugs(33, 5, ACCEL VPORT REWRITE: '  http-uri  ');
 }
 }
 
 static void
 

Re: [PATCH] rock fixes and improvements

2014-04-28 Thread Amos Jeffries
On 27/04/2014 6:09 a.m., Alex Rousskov wrote:
 Hello,
 
 The attached patch contains several fixes and improvements of the
 shared memory and rock disk caching code, some of them quite important.
 Here is the summary (detailed change descriptions are quoted at the end
 of this message):
 

in DiskIO/IpcIo/IpcIoFile.cc:
 * please use SBuf for DbName global
 * please remove HERE in chunk at line 888:
  - debugs(79,3, HERE  rock db opened  ...


in MemStore.cc:
 * consider making ExtrasLabel and SpaceLabel into SBuf. same in
Transients.cc


in Store.h:
 * new documentation for checkCacheable() is unclear. It only made sense
after reading the related functions descriptions and some code.
 - Consider whether generally cacheable as the brief and expanding the
brackets to separate sentences.
 - please consider making the function return bool for even better

 * please use // TODO (non-doxygen slashes) for private TODOs, or
proper doxygen /// \todo tag for ones that to be published in the code
manual TODO page.


in src/ipc/StoreMap.cc:
* Missing decrement?
@@ -64,7 +74,7 @@
---shared-count;
+anchors-count;


Amos


Re: [PATCH] cache_peer standby=N

2014-04-28 Thread Alex Rousskov
On 04/27/2014 10:02 PM, Amos Jeffries wrote:

 We should state the problem with idles clearly (yes it is difficult to
 word),

We already do that:

 + max-conn limit works poorly when there is a relatively
 + large number of idle persistent connections with the
 + peer because the limiting code does not know that
 + Squid can often reuse some of those idle connections.

Do you want us to add This poor idle connection management is a
problem. sentence to the above?



 or we fix that problem (see below) and update the documentation

The change is not trivial, so I do not think we should be forced to do
that as a part of this project. There are many problems with idle
connections, and we are not making them worse by adding the standby
pools, quite the opposite. It feels like we are being penalized for
improving documentation of ancient problems.


Alex.



Re: [PATCH] ConnStateData flexible transport support

2014-04-28 Thread Alex Rousskov
On 04/28/2014 07:10 AM, Amos Jeffries wrote:

 * ssl-bump transforms the transportVersion from whatever it was
 previously (usually HTTP or HTTPS) to HTTPS.

 +/// the transport protocol currently being spoken on this connection
 +AnyP::ProtocolVersion transportVersion;


If I recall our earlier discussions correctly, transport is TCP, UDP,
and such while HTTP and HTTPS are transfer protocols. It sounds like
you want to use the new data member for transfer, not transport
protocols. If yes, the member should be renamed accordingly.

Also, if this is often about the protocol name rather than just its
version, let's remove Version from the data member name (PortCfg
already uses that approach).

Finally, the patch does not actually use the version part of the
transportVersion data member AFAICT. Perhaps the data member type should
be changed from ProtocolVersion to ProtocolType?


 -debugs(33, 5, HERE  converting   clientConnection   to SSL);
 +debugs(33, 5, converting   clientConnection   to SSL);

To reduce merge conflicts, please do not remove HERE from debugging
lines that otherwise do not need to be changed.


 This variable can
 be altered whenever necessary to cause an on-wire protocol change.

Altering the data member does not cause an on-wire protocol change in
the patched code AFAICT. Perhaps you meant that the data member should
always reflect the current wire protocol?


Cheers,

Alex.