Re: [MERGE] c++-refactor HttpHdrCc

If you guys are OK, I'll merge the current implementation, then we can
alter it post-merge.

Right now what Squid does is A1->B2 for all directives except for
max-stale, for which it does B4 (forward valueless). This is probably
the worst thing that can be done.

IMHVO the best thing we could do is forward things we can't parse
as-is (A2-B3). If we change nothing we can probably also keep a copy
of the raw directives, saves some cycles to reassemble it at pack-time
.

On Tue, Oct 4, 2011 at 4:49 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On Mon, 03 Oct 2011 18:02:38 -0600, Alex Rousskov wrote:
>>
>> On 09/30/2011 09:30 PM, Amos Jeffries wrote:
>>>
>>> On Thu, 29 Sep 2011 17:59:13 +0200, Kinkie wrote:
>>>>
>>>> On Thu, Sep 29, 2011 at 5:55 PM, Alex Rousskov wrote:
>>
>>>>>>> We got a malformed max-stale value. We have only
>>>>>>> two options when it comes to that value interpretation, I think:
>>>>>>>
>>>>>>>   A1. Treat it as valueless max-stale.
>>>>>>>   A2. Ignore it completely.
>>>>>>>
>>>>>>> Since valueless max-stale results in possibly very stale content
>>>>>>> being
>>>>>>> served to an unsuspecting client, I think #2 is much safer (and
>>>>>>> compliant with HTTP).
>>
>>>>>>> BTW, we have three options when it comes to forwarding the malformed
>>>>>>> directive:
>>>>>>>
>>>>>>>   B1. Forward our own valid directive.
>>>>>>>   B2. Forward nothing.
>>>>>>>   B3. Forward the malformed directive.
>>>>>>>
>>>>>>> While option B3 could be the best, it requires more parsing changes.
>>>>>>> Option B2 is much better than B1, IMO, and requires minimal changes.
>>>>>>>
>>>>>>> We currently implement A1 and B1.
>>
>>
>> Hi Amos,
>>
>>    I sense misconmmunication. Please keep in mind that A* options are
>> concerned with internal interpretation of a malformed CC directive by
>> Squid while B* options are concerned with forwarding of a malformed CC
>> directive by Squid. We treat those two areas separately, and we do not
>> discuss a whole CC header.
>
> Understood.
>
>>
>>
>>> B2 is not a valid option AFAIK.
>>
>> Why not? We can drop a CC directive if we consider it malformed, I
>> think. And if it is the only directive, we would drop the whole CC
>> header, of course.
>>
>> In fact, don't we already implement B2 for all other malformed
>> integer-valued CC directives?
>
> Possibly. This does not make it a full case though. It seems to be one
> instance of B1 where our fixed valid value happens to be nil.
>
> ie  A1->B2 is not valid.  and for the reasons outlined below A2->B2 is not
> valid.
>
>>
>>> HTTP does not offer the option A2 as you are discussing right now.
>>> Since we know the header we must either treat it as an option known (A1)
>>> or something unknown (A3).
>>
>> A2 is "ignore it", which (from interpretation point of view) is
>> equivalent to "treat it as something unknown" because Squid ignores
>> unknown CC directives.
>>
>>
>>>  * A3 means we can ignore it for operational purposes (like A2), the
>>> natural result being B3.
>>
>> I do not understand the difference between A2 and A3. A2 does not imply
>> B3 though. A2 implies (either B2 or B3).
>
> Possible miscommunication here. I was reading your A* as what to do with the
> values from the point of parser sighting them onwards.
>
> A2 as I see it means produce 'other' key-value token from the parse.
> Ignoring it internally. The only compliant action on output is B3. This
> could lead to accepting a second instance of max-stale as the real
> max-stale. So I'm kind of against choosing this action.
>
>
> A3 as I see it means parse and accept as a unknown *non-nil* value of
> max-stale. Treating it internally as valueless (or zero value to be very
> conservative?), BUT the compliant output is B3. Without the problems of
> accidentally emitting 2 max-stale values, because even if ignored we record
> in the state that a max-stale was seen.
>
>
>>
>>> A1 means we have to decide betrween B1 or B3. Treating it as a known
>>> directive which has been extended with some unknown extension value
>>> (B3). Or as definitely malformed and invalid (B1).
>>
>> A1 can be combined with all three Bs.  I would not recommend A1 at all
>> though because we may end up serving stale content to a client that does
>> not want it. If we have to gamble, it is better to server "too fresh"
>> than "too stale" content, IMO (it is also compliant!).
>
> The new updated RFC texts from HTTPbis are now stating:
>
> "
>   A proxy, whether or not it implements a cache, MUST pass cache
>   directives through in forwarded messages, regardless of their
>   significance to that application, since the directives might be
>   applicable to all recipients along the request/response chain.  It is
>   not possible to target a directive to a specific cache.
> "
>
> So we do not have B2 as a compliant option. If we receive it, it MUST be
> relayed.
>
> Erasing it could be an option, but not a compliant one. Striping the
> unknown-value to make it less strict than we received would be more likely
> to add problems in downstream software which might understand it.
>
> That said, the spec clearly outlines it as delta-seconds so if its not
> parseable as delta-seconds its clearly malformed and B1 probably should not
> pass "max-stale".
>
>
> Which leaves us in a mess because;
>  A1->B1 seems intuitively the right thing, but has bad side effects.
>  A1->B3 seems like a reasonable alternative to avoid the effects, but
> gambles on delta-seconds never changing.
>
>
>
> I think we might have to get around this by using the max_stale directive
> (or refresh_pattern override value) to limit the max-stale value relayed
> upstream from the current Squid. If we consider this to be an administrative
> override determining that this Squid will not emit objects older than
> max_stale its reasonable to shrink the requested staleness range from the
> valueless "anything" and ensure we get an object as current as we want,
> within the wider range of what the client wants.
>
> Amos
>
>

-- 
    /kinkie