[sipcore] AD review: draft-ietf-sipcore-event-rate-control-03

[Robert Sparks <[e-mail]>]

Summary: This draft has a few adjustments that are needed before moving it into IETF Last Call.

Major question:

Why isn't this an Update to 3265? Is there text here that prevents a subscriber
from generating Event headers in 200 OKs to NOTIFYs mid-subscription (when he
didn't probe for support using the SUBSCRIBE?) How would they know the request
got honored?  The possibility of running into implementations that break should
be called out.  4.2 indicates the subscriber only gets a "hint" about support
for rate-control in the notifier - is the condition it describes really only a
hint?

In several places, the notifier is given permission to adjust an interval based
on local policy.  The document should be explicit about allowing the adjustment
in any direction (increasing or decreasing) since there are so many other uses
of intervals in SIP and SIP Events that allow adjustment only in one direction.
A few places I noted when reading the document were the Note in REQ7, 4.3 4th
paragraph, 5.2 paragraph 3, 6.2 paragraph 3.

The last paragraph of section 3.6 claims "exactly the same properties" except
for being generated constrained to a schedule. Can you clarify which properties
you mean? Many properties of the notifications beside their timing are clearly
different (for instance, you may miss state transitions).

The security considerations section deserves more text: 
* What is the forward reference from section 3.4 supposed to be pointing to?
* Call out the implications on a Notifier having to store/aggregate partial state
* Note that the Event header (particularly in 200 OKs) is not integrity protected. 
  This would allow anything that could modify the message in flight (or an 
  eavesdropper that could race a 200 OK in) to suppress (or flood) notifications 
  without the subscriber seeing what caused it.

The assertion that applying rate limiting and compression together results in
savings as good as the sum of applying them independently should be supported
or adjusted. I think it's sufficient to say they can be applied together.

Below are several suggestions for text tweaks. The first few (staring with *)
are the most important. 

* Section 3.2 paragraph 4: suggest replacing "does not typically" with "may not"

* Section 3.2 last paragraph: The sentence 'The "max-interval" parameter 
      indicates ... complete state information' is difficult to parse. Could it
      be simplified?

* Section 4.3 first paragraph, last sentence: "For such cases" is ambiguous.
  Suggest "If the min-interval value is greater than the subscription expiry".

* Section 6.2 last paragraph: This currently says the timeout mechanism does
  not affect when 3261 transaction retransmissions are generated. It should
  also explicitly note that retransmissions do not affect the calculation of
  the next timeout.

Introduction, paragraph 2: suggest replacing "congestion" with "load"

Section 3.1  paragraph 3: suggest replacing "amount of traffic" with 
"number of notifications"

Section 3.5 paragraph 1: Suggest a reference for RLS after "list subscription".
The sentence "Moreover, the list event notifier..." should be more explicit
about using the rate mechanism for any back-end subscriptions it might have.

Suggest referencing 3261 in the last paragraph of 4.2

Section 4.3, 3rd paragraph last sentence. The only way the subscriber _can_
resume notifications is to renew the subscription with a resubscribe request.
Would this text work? "This results in receiving no further notifications until
the subscription expires or the subscriber sends a SUBSCRIBE request refreshing
the subscription (perhaps resuming notifications)".

The text needs to be adjusted to reflect subnot-etags being issued as an RFC

Adam suggested some RFC-Editor notes in the proto writeup (which may address
some of the above comments). Please be sure to incorporate those when revising
the draft.

One last question:

If the combination of min-interval, max-interval, and average-interval make
little sense, why does the document allow them to be combined? I think what the
group was trying to say is that we currently don't forsee a use for combining
those options, but do not wish to forbid their combination.