Re: [sipcore] Review of draft-ietf-sipcore-keep-05

[Christer Holmberg <[e-mail]>]

Hi Dale, 

>Only the items for which I have further comment:
> 
>>From: Christer Holmberg [[e-mail]]
> 
>One general point that might be worth adding to section 1 is, 
>"This specification is an extension of SIP Outbound and is to 
>be read based on [RFC5626]."  A considerable number of my 
>comments and confusions would have been eliminated had I 
>understood that this draft can only be effectively read given 
>an understanding of 5626.

I think *extension* is the wrong wording.

The draft defines a negotiation mechanism for the keep-alive mechanism, which is defined in the Outbound spec (at some point I think we said that the keep-alive mechanism should have been in a separate spec from the rest of Outbound in the first place, but it was too late to make that change at that point). 

Would it be more clear if I added the following sentence to section 1?

"The details of the keep-alive mechanisms are described in [RFC 5626]."

Regards,

Christer






> 
> > >Section 3
> > >
> > >"Edge proxy"
> > >
> > >It's not clear that "edge proxy" has any normative significance in 
> > >this I-D, as this mechanism can be used between any two entities.
> > >It seems that this definition and the accompanying Note could be 
> > >removed at no loss.  (The definition needs revision, also.)
> > 
> > Section 5 talks about the edge proxy, where it describes that the 
> > usage of Flow-Timer is only defined between a UA and an 
> edge proxy, so 
> > I think it is useful to have the definition.
> 
> OK, I can see the usefulness of that.
> 
>    Edge proxy: As defined in [RFC5626], a SIP proxy that is located
>    topologically between the registering User Agent (UA) and the
>    Authoritative Proxy.
> 
> One change that I think would have helped me avoid confusion 
> would be to change "a SIP proxy" to "any SIP proxy".  Because 
> naively I think of "edge proxy" as meaning only the one proxy 
> closest to the UA; but in Outbound, it refers to any proxy 
> between the Authoritative Proxy and the UA, there can be many 
> of them for a single UA.  The phrase "any SIP proxy" warns me 
> that there might be more than one in a particular 
> UA/Registrar connection.  OTOH, if we warn the reader to read 
> RFC 5626 first, that probably doesn't matter.
> 
> > I guess I could also add the following sentence to section 1:
> > 
> >         "In addition, [RFC5626] only allows keep-alives to be
> >         negotiated between a UA and an edge proxy, and not 
> between other
> >         SIP entities."
> 
> That certainly is a significant restriction on Outbound.
> 
> > >'"keep" parameter'
> > >
> > >Strictly speaking, this entry is a description not a 
> definition; the 
> > >phrase '"keep" parameter' identifies the parameter 
> unambiguously.  It 
> > >would be better to remove this entry and add its information to 
> > >section 4.1, which needs to be a bit more explicit about the 
> > >mechanism.  (See below.)
> > 
> > The definition text used to be even longer, but parts were removed. 
> > However, the terminology is used in the documents, so I think it is 
> > good to have it in the definitions, instead of writing "the 
> Via header 
> > field "keep" parameter" everywhere.
> 
> OK, now I see what the point of the definition is.  In that 
> case, you can say:
> 
>    "keep" parameter: The "keep" header field parameter of the topmost
>    Via header of a SIP message.
> 
> That defines the phrase exactly; it's shorter (which people 
> seem to want); and it removes the text which describes how 
> the parameter is used and leaves only the definition itself.
> 
> > >   A SIP entity indicates willingness to send keep-alives 
> towards the
> > >   adjacent downstream SIP entity using SIP requests by applying a
> > >   "keep" header parameter to the topmost Via (which is 
> the Via that
> > >   it inserts).  The associated response is used by the adjacent
> > >   downstream SIP entity to indicate willingness to receive  
> > > keep-alives by applying to that topmost Via a "keep" header
> > >   parameter with an integer value.  The integer value, if 
> not zero,
> > >   indicates the keep-alive frequency that is recommended by the
> > >   adjacent downstream SIP entity.
> > >
> > >That gives the outline of the mechanism in one paragraph.
> > 
> > During WGLC, there were comments/discussions about removing 
> text, and 
> > making it shorter and easier to read. I don't want to start adding 
> > text again at this point, unless something is unclear or wrong.
> > 
> > Also, in this case, section 4.1 is a general overview section. The 
> > details on how to indicate willingness to send and receive 
> keep-alives 
> > are described in sections 4.3. and 4.4, so there is no need 
> to repeat 
> > the same text in 4.1.
> 
> The current text was unclear to me in that while it said that 
> keepalives were negotiated via requests and responses it gave 
> no hint of *how*.  If brevity is important, I would mention 
> the specific use of "keep" in the request and response, and 
> leave the discussion of frequency to the more detailed sections.
> 
> > I would not like to start adding normative text on how and when 
> > entities shall use the mechanism, because it might depend on the 
> > entity types, and the environments where they are deployed.
> > 
> > However, I guess we could add some general guideline text:
> > 
> >         "In general, it can be useful for SIP entities to indicate
> >         willingness to send keep-alives, even if they are not aware
> >         of any necessity for them to send keep-alives, since the
> >         adjacent downstream SIP entity might have knowledge about
> >         the necessity. Similarly, it can be useful for SIP entities
> >         to indicate willingness to receive keep-alives, even if they
> >         are not aware of any necessity for the adjacent upstream SIP
> >         entity to send them."
> 
> I would add a final clause to the second sentence like the 
> final clause of the first sentence, "... since the adjacent 
> upstream SIP entity might have knowledge about the 
> necessity."  I agree that this should not be normative, but 
> it seems to me to be something that the reader should be 
> warned about, since it's not at all obvious at first reading 
> that both devices should negotiate the possibility of 
> keep-alives even if nether knows that they are needed.
> 
> > The text refers to RFC 5626 regarding the STUN and CRLF keep-alive 
> > messages, and RFC 5626 defines when each is used.
> >
> > Also, remember that the purpose of the daft is not to change the 
> > keep-alive mechanism in RFC 5626, only to provide a mechanism to 
> > explicitly negotiate the usage of it, so we should not repeat 
> > mechanism text that is already defined in RFC 5626.
> > 
> > BUT, in order to try to make everyone happy, I could add 
> the following sentence to the first section of 4.3:
> > 
> >         "[RFC5626] defines when to use STUN, respectively 
> CRLF, for keep-alives."
> 
> This is an example of where this draft is really an extension 
> of RFC 5626, which is something you understand implicitly and 
> I didn't know.
> I think that confusion can be cured by the note in section 1 
> that I mention above.
> 
> > Maybe we could simply say:
> > 
> >         "This specification does not specify actions to take if
> >         negotiated keep-alives are not received. As defined in
> >         [RFC5626], the receiving SIP entity may consider a
> >         connection to be dead in such situations."
> 
> Again, this draft is just a way of activating the mechanism 
> defined in 5626.
> 
> > My understanding of 5626 is that only an edge proxy can add the 
> > Flow-Timer header field, since Outbound only defines the usage of 
> > keep-alives between the UA and edge proxy.
> > 
> > Outbound itself wouldn't work if multiple entities were allowed to 
> > insert Flow-Timer.
> 
> The critical thing is that since only an edge proxy, and only a
> *first* edge proxy, can add a Flow-Timer header, you will 
> never have an entity adding "keep=n" to a response which 
> *already* has a Flow-Timer header, although it might add both 
> "keep=n" and Flow-Timer to a response.  If one hasn't read 
> 5626, this isn't obvious.
> 
> > The mechanism allows proxy-to-proxy keep-alives, and there 
> also used 
> > to be an example (or, at least a use-case description) about 
> > proxy-to-proxy. However, it was removed, because people confused it 
> > with proxy-to-proxy heartbeats, which is a different thing than 
> > keep-alives.
> 
> Yes, it probably isn't worth the confusion to describe the 
> proxy-to-proxy case, which will probably be uncommon.
> 
> Dale
>