Peer-to-Peer (P2P) Overlay Diagnostics
draft-ietf-p2psip-diagnostics-22

For Sections 9.1 and 9.2, I wish there had been some working group discussion that resulted in the decision to make the registry policies Standards Action.  It seems that some softer policy, such as "IETF Review" (which might allow for registrations from Experimental documents) or Specification Required (which would allow review by a designated expert of a non-RFC specification) would work OK for this registry, and I would have liked the working group to have actively considered that.  But that ship has sailed for this document and this working group, and it's not likely to box us in for this case, so this is now a non-blocking comment.  Thanks for the discussion we've had on this point.

In addition to what Ben has already said...

One of the things that idnits calls out is this:
  -- The draft header indicates that this document updates RFC6940, but the
     abstract doesn't seem to mention this, which it should.

I believe it's not a problem that the abstract doesn't mention it, but one *reason* the abstract doesn't mention it is that the rest of the document doesn't mention it either.  It's not at all clear WHY this document updates 6940.  Why?  (This is in support of Ben's comment, as well as being a question to the shepherd.)

The idnits tool also mentions the pre-5378 disclaimer, and this has been resolved, so the disclaimer should be removed when the draft is updated.

I strongly agree with Ben's comment about needing explanations for a number of SHOULDs (and SHOULD NOTs) in the document.  RFC 2119 says that for SHOULD, "the full implications must be understood and carefully weighed before choosing a different course."  Without any explanation, there's no way for implementors to understand the implications and to weigh anything, and I tripped over that quite a number of times during my review.

I agree with Spencer's comment that we don't usually strong-arm IANA with 2119 key words.  It's a small point, and I don't think IANA are easily offended [ :-) ], but "IANA is asked to create" is a better approach than "IANA SHALL create", and so on.

General comments:

The draft styles itself as P2PSIP diagnostics. But P2PSIP is a usage of RELOAD, and the draft mentions it is generally applicable to RELOAD. This is misleading, and might artificially limit it's applicability.  Perhaps this should be called RELOAD diagnostics? Or if it is really specific to P2PSIP, that should be clarified.

The headers show that this draft updates RFC 6940. As far as I can tell, it extends 6940 along defined extension points. If so, that's not really an update. (Also, the shepherd write up says it does not update anything.)

Specific comments:

- 4.1, 2nd to last paragraph: "The default policy or access rule for a type of diagnostic information is "permit" unless specified in the diagnostics extension document."

Can you elaborate on this? I assume "diagnostic extension document" refers to a spec that registers something in one of the new registries defined herein?  How does a policy of "permit" interact with the authorization requirements and security considerations elsewhere in the draft?

-6.2:
The section has lots of SHOULDs, where the reasons they are not MUSTs are not obvious. Please offer reasons why an implementation might choose not to follow the SHOULD.

- 6.4, first and last paragraphs:
The MAYs seems like a statements of fact rather than normative requirements.

-6.4, 2nd paragraph:
Does this document have requirements for clock resolution or skew? (It seems odd to mention the NTP capabilities unless they matter.)

9.1 (and others in the IANA section)

Where should these new registries be created? (I imagine IANA can figure it out, but it's best to be explicit.)

Editorial:

- 4.1, 2nd paragraph:
OLD:
Essentially, this document reuses RELOAD [RFC6940] specification and extends them to introduce the new diagnostics methods.
NEW:
Essentially, this document reuses the RELOAD [RFC6940] specification and extends it to introduce the new diagnostics methods.
END.

I agree with Ben, Joel & Mehmet (OPS DIR), and Barry.

On top of that, 
- This document looks to me as a specific implementation, written in a draft format.
- While reading the document, I was wondering: Is this P2P specific or P2PSIP specific?
Maybe a missed opportunity to have generic OAM principles for P2P, with RELOAD extension.
- Finally, any connection with LIME, at least on the presentation aspects.

I agree with Ben and Barry that the "Updates RFC 6940" is not well-justified.  I also agree with Joel that the readability makes it hard to completely understand what is being proposed.

Thanks for improving the IANA considerations section! However, Section 9.1 still has a bug that IMHO needs to be corrected. The reserved values need to be shifted in value by one, because now you have two with the value 1.

As Alexey said: "The draft has improved, however Section 9.1 still seems incorrect: if the first bit is reserved, then the first allocated value must be 2, so all other allocated values should be shifted by 1 bit."

However, I have cleared so that the shepherding AD and the WG can take care of this. Please make sure you fix this though, as my understanding is that if you didn't make the change, the protocol would not work as you intended it to work.

I was slightly surprised by "IANA SHALL" do things. Don't we usually do IANA considerations without RFC 2119 language?

- section 3a.: what kind of crypto processing overhead would
make a node unresponsive? Seems a bit odd to me.

- 4.1: the para that starts with "The diagnostic information can
only be provided to authorized nodes." seems pointless to me and
could be deleted.

- 5.3: STATUS_INFO - I don't get the benefit of half-specifying
a thing like this; PROCESS_POWER is also under specified.

- 5.3, SOFTWARE_VERSION etc: providing this information ought not
be on by default - even if the overlay config calls for it to be
available to node X each node can and should decide itself
whether or not to send. While you do say (in 6.3 and 7 and 8)
that nodes need to check that the requestor is authorized to get
this information, I think you ought also say that the default is
to not send. (This is not a discuss because I think you have
enough of those;-)