Re: /bzr/squid3/trunk/ r13384: Bug 1961: pt1: URL handling redesign

On 1/05/2014 10:21 p.m., Tsantilas Christos wrote:
> On 04/30/2014 09:33 PM, Alex Rousskov wrote:
>> On 28/04/2014 5:35 a.m., Tsantilas Christos wrote:
>>> Unfortunately this is not build with ecap.
>>
>>
>> On 04/27/2014 07:57 PM, Amos Jeffries wrote:
>>> What the eCAP field needs to be set to depends on its definition:
>>
>>
>> libecap::FirstLine::protocol() is meant for things like
>>
>> * the HTTP-Version part of HTTP messages (in RFC 2616 terminology) or
>> * the ICAP-Version part of ICAP messages (in RFC 3507 terminology).
>>
>> It is not related to the URI. In fact, libecap::FirstLine does not know
>> about the message URI! Only its libecap::RequestLine child knows that
>> requests have URIs.
>
> OK, this is clarifies what we should do for ecap.
>
> Before the patch r13384 for the libecap::FirstLine::protocol() returned
> the protocol information from url, which now looks wrong.
>
>>
>> HttpMsg::http_ver in recent trunk is described as being meant for the
>> same thing. Thus, there is a perfect match between the two concepts.
>>
>>
>> Now, we need to understand how the actual code maps to the above design,
>> and what code changes are needed to fix the trunk build broken by
>> r13384. My understanding is that
>>
>> 1. If Squid trunk does not set HttpMsg::http_ver [correctly] when
>> parsing any requests or responses, we should fix the corresponding Squid
>> code. This will ensure that eCAP adapters are getting the right
>> information about virgin messages. This item requires an investigation
>> and may not require any code changes.
>
> If I am not wrong, currently the protocol in HttpMsg::http_ver is
> always AnyP::PROTO_HTTP.
> I think this is normal because we are always handling HTTP messages.
>

You are right. eCap not using the version min.maj values should mean we
dont have to worry about incorrect versions for this fix. Even with the
wrong version set or invalid default value it will have the "HTTP" part.

eCAP should also see "ICY" on those responses. But not other protocol
names yet.

My progress so far in the checking I have found:

* HttpStateData::processReplyHeader() is not storing anything into
http_ver. Needs this at line 765:

+ newrep->http_ver = newrep->sline.protocol;
     newrep->removeStaleWarnings();

==> this is a bug preventing "ICY" being sent to ICAP/eCAP.

* Adaptation::Icap::ModXact::encapsulateHead() is not copying it on
adapted replies. Needs this at line 1536:

        HttpReply::Pointer new_reply(new HttpReply);
        new_reply->sline = old_reply->sline;
+ new_reply->http_ver = old_reply->http_ver;
        headClone = new_reply.getRaw();

* Adaptation::Icap::ModXact::prepEchoing() does not copy the http_ver
value. It appears to parse only the mime headers portion of the ICAP output.
 - I belive this means it is setting the adapted request/reply to
"HTTP/0.0" default and ignoring any request-line/status-line changes the
ICAP service tries to make.

* httpsEstablish() fakeRequest is never setting http_ver. Needs this at
line 3582:

        HttpRequest::Pointer fakeRequest(new HttpRequest);
+ fakeRequest->http_ver = Http::ProtocolVersion(1,1);
        fakeRequest->SetHost(details->local.toStr(buf, sizeof(buf)));
        fakeRequest->port = details->local.port();

* Same for httpsAccept() fake request.

* Same for PeerPoolMgr::start() probe request

* code using "new HttpReply" does not set http_ver. A few places set
sline.protocol to the proper value, but ignore http_ver.

For completeness the successful checks were:

 * client request parser - http_ver is set properly.

 * redirected requests - http_ver is copied properly.

 * adapted requests - http_ver is cloned properly into the default
adapted request object.

 * HttpRequest::clone() - http_ver is copied correctly.

 * HttpReply::clone() - http_ver is copied correctly.

TODO: http_ver is not set when created by any call to urlParse() which
generates a new request object. This needs checking whether the callers
set it.

TODO: I have not yet tracked down exactly what code is doing the
first-line parse for the encapsulated ICAP requests or replies. So am
not certain the cloned value is being overwritten properly with ICAP
sent values.
  -> Is prepEchoing() behaviour a hint that there is no such code?

I am thinking at this point that we should change the constructor on new
HttpRequest/HttpReply to set default of HTTP/1.1 matching Squids
official version. Then only touch http_ver when cloning/copying objects
or parsing external inputs.

>>
>> 2. If Squid trunk does not use HttpMsg::http_ver [correctly] when
>> formatting any requests or responses, we should fix the corresponding
>> Squid code. This will ensure that eCAP adapters can adapt virgin
>> messages as needed. Note that Squid does not support post-cache
>> vectoring points so Squid should still set its own http_ver in any
>> outgoing messages. This item requires an investigation and may not
>> require any code changes.
>>
>> 3. Squid eCAP implementation should be changed to read and write
>> HttpMsg::http_ver. This affects two
>> Adaptation::Ecap::FirstLineRep::protocol methods and possibly other
>> code. This item requires code changes.
>
> Maybe we need to modify:
> - Adaptation::Ecap::FirstLineRep::protocol to return the
> HttpMsg::http_ver protocol information
> - Adaptation::Ecap::FirstLineRep::protocol(const Name &p) to set
> HttpMsg::http_ver

Definitely for these ones.

> - Adaptation::Ecap::RequestLineRep::protocol to return the url protocol
> information.
> - Adaptation::Ecap::RequestLineRep::protocol(const Name &p) to set
> HttpRequest url protocol information (or something equivalent).
>
> The Adaptation::Ecap::RequestLineRep::protocol isn't it referring to the
> URL protocol (http://, https://, ftp://, etc)?
> Else eCAP services need to re-parse URL to retrieve this information...
>

Amos
Received on Thu May 01 2014 - 13:36:50 MDT