Subject: [P2PSIP] WGLC for draft-ietf-p2psip-diagnostics-09

From: "Rosen, Brian" <[email-masked]>
Date: Mon, 17 Sep 2012 13:18:44 -0400

We are issuing a two week Working Group Last Call for draft-ietf-p2psip-diagnostics-09.

Please send comments to the list, including any statements you may have about the readiness of this document to be published.

WGLC will close on October 2.

Brian & Carlos

---

From: Zongning <[email-masked]>
Date: Tue, 18 Sep 2012 00:11:40 +0000

I have read the diagnostic draft and think it is useful and well written. I support moving it forward.

-Ning

> -----Original Message-----
> From: [email-masked] [mailto:[email-masked]] On Behalf Of
> Rosen, Brian
> Sent: Tuesday, September 18, 2012 1:19 AM
> To: P2PSIP WG
> Subject: [P2PSIP] WGLC for draft-ietf-p2psip-diagnostics-09
> 
> Please send comments to the list, including any statements you may have
> about the readiness of this document to be published.
> 
> _______________________________________________
> P2PSIP mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/p2psip

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Tue, 25 Sep 2012 09:52:28 -0700

I carefully read this I-D, and I found a lot of nits, which make it difficult
to read.  I think that this draft is ready to be submitted to the IESG, but I
would like to start a discussion on one specific point, security.

The draft is very light on the discussion about who is authorized to do what.
 Obviously some of the information that can be requested are sensitive, and I
am afraid that without having a mechanism for authorization in the document,
implementers will be reluctant to add it to their implementation.  I think
especially of SOFTWARE_VERSION, but there can be future Diagnostics Kinds that
are not for general availability.  So I would suggest to define in the spec an
additional mechanism to restrict the access to some Kinds:

The idea would be to add an extension in the configuration file to authorize
access to these Diagnostics Kinds only to a specific list of Node-IDs (in a
similar way that signer node-IDs are authorized to sign kind and configuration
elements in signer and king-signer elements.

Nits
====

- - The title, abstract and section 1 uses the acronym "P2PSIP", but in fact
what is described is not limited to P2PSIP (i.e. the usage of
draft-ietf-p2psip-sip) but can be applied to any usage of RELOAD.  So I would
recommend to change P2PSIP to P2P.

- - Section 4:  Path_Track

This is used inconsistently in the whole document.  I would suggest to change
it to PathTrack everywhere, with the request structure named PathTrackReq and
the answer PathTrackAns.  The message code names (path_track_req and
path_track_ans) are correct.

- - The meaning of peer, node and client is well defined in
draft-ietf-p2psip-base, but the usage of these words is not consistent in this
document.  I would suggest to check every instance and change them if needed.
 For example in Section 4, last paragraph, "... must be only provided to
authorized peers."  should be "... must be only provided to authorized
nodes.", and so on...

- - Section 4.2, last paragraph:  "There have been proposal on the list..."

This kind of language probably does not have its place in a normative section.
 Move this to an annex.

- - Section 4.3:  Move to an Annex (see above)

- - Section 5.1.1, title:

s/DiagnosticRequest/DiagnosticsRequest/

- - Section 5.1.1, structure:

I think that this would make it clearer that there can be more than one
DiagnosticExtension:

enum { (2^16-1) } DiagnosticExtensionRequestType;

struct {
    DiagnosticExtensionRequestType type;
    opaque  diagnostic_extension_contents<0..2^32-1>;
} DiagnosticExtension;

struct {
    uint64 expiration;
    uint64 timestamp_initiated;
    uint64 dMFlags;
    DiagnosticExtension diagnostic_extensions<0..2^32-1>;
} DiagnosticsRequest;

- - Section 5.1.1, 7th paragraph "the flags can be extended by standard action."

Add a cross reference to the correct subsection in section 9 (IANA
Considerations).

- - Section 5.1.2, title:

s/DiagnosticResponse/DiagnosticsResponse/

- - Section 5.1.3, second paragraph:

s/and other bits Must be cleared/nd other bits must be cleared/

- - Section 5.1.3:

I would suggest to provide the structure (in the presentation language defined
by RELOAD section 6.3.1) for all the types defined in this section, if
interoperability between implementations is wanted.

- - Section 5.1.3: SOFTWARE_VERSION:  I would suggest to change the example to
something more anonymous.  As for the format itself, this draft may be of some
help:

http://www.ietf.org/internet-drafts/draft-karcz-uuas-00.txt

- - Section 5.1.3: MACHINE_UPTIME

s/the nodes has been/the node has been/

- - Section 5.1.3: MEMORY-FOOTPRINT

s/32 -bit/32-bit/

- - Section 5.1.3: INSTANCES_STORED

s/The array is index by/The array is indexed by/

- - Section 5.1.3: UNDERLAY_HOP

s/for diagnostics purpose./for diagnostics purpose)./

- - Section 5.1.4, 2nd paragraph

s/Editor's Note: The self-tuning/The self-tuning/

- - Section 5.2, second paragraph

s/Section 5.3.3/Section 6.3.3/

- - Section 5.2, third paragraph: "If a peer does not extend the extension,..."

Does not make sense.

- - Section 5.3, second paragraph

s/Route_Query/RouteQuery/

- - Section 5.3.1

s/compressed id/opaque ID/

- - Section 5.5.1, first paragraph, "The sender MUST include the dMFlags field"

This field is not longer optional, so this text is useless.

- - Section 5.5.1, first paragraph, "...but MUST include the field"

Same than above.

- - Section 5.5.1, second paragraph

s/diagnostic ping/diagnostic Ping/

- - Section 5.5.1, 2nd and 3rd paragraphs:

s/NodeId/Node-ID/
s/ResourceID/Resource-ID/

- - Section 5.5.2, second paragraph "contain the Error code followed by the
subcode..."

What is this subcode?

- - Section 5.5.3, 3rd paragraph "will represent 100-hops"

Where does this 100-hops come from?

- - Section 5.5.3, 5th paragraph "subcode and description"

What is this subcode?

- - Section 5.5.3, 5th paragraph

s/with the Error Code 1 "Error_Unauthorized"./with the Error Code 2
Error_Forbidden./

- - Section 5.5.4, 2nd paragraph:

Remove this paragraph.

- - Section 5.5.4, 3rd paragraph:

s/Inspect/PathTrack/

- - Section 6.1

s/WEMA_BYTES_RCVD/EWMA_BYTES_RCVD/

- - Section 6.2

s/StatusInfo/STATUS_INFO/

- - Section 6.3

s/Inspect/PathTrack/
s/InspectReq/PathTrackReq/

- - Section 9

I find the way the registries are organized makes them difficult to understand.

It would be better to have one registry merging 9.1, 9.2 and 9.6, with the
following columns:

- - Diagnostic Kind Type (from 9.2, same than diagnostic information from 9.6)
- - diagnostic flag (from 9.6, can be empty)
- - Code (from 9.2)
- - Diagnostic Extension Name (from 9.1, can be empty)
- - Code (from 9.1, empty if the previous cell is empty)
- - Specification (from 9.1, 9.2 and 9.6)

Another alternative is to have one registry for the dMFlags kinds, and another
for the extensions Kinds, and perhaps use different RFC 5226 requirements.

Private and experimental uses should also be documented.

> WGLC will close on October 2.

- -- 
Marc Petit-Huguenin
Email: [email-masked]
Blog: http://blog.marc.petit-huguenin.org
Profile: http://www.linkedin.com/in/petithug

---

From: Songhaibin <[email-masked]>
Date: Fri, 28 Sep 2012 01:04:34 +0000

Hi Marc,

Thank you very much for the feedback. See inline.

> Sent: Wednesday, September 26, 2012 12:52 AM
> To: Rosen, Brian
> Cc: P2PSIP WG
> Subject: Re: [P2PSIP] WGLC for draft-ietf-p2psip-diagnostics-09
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 

Thanks, the document will double checked and updated.

> implementers will be reluctant to add it to their implementation.  

Good point!

>I think
> are not for general availability.
  So I would suggest to define in the spec an
> elements in signer and king-signer elements.

This should work. Could this lead to the over dynamic of the configuration file? Another option is for each Kind, just stored these signed IDs as a value in the overlay.

BR,
-Haibin

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.12 (GNU/Linux)
> 
> iQIcBAEBCAAGBQJQYeFKAAoJECnERZXWan7E68kQAJamA99TW0I/MdPwrOEzjEBI
> kX+qcAiDr6UTWwMaUfwq/gbVH2WHz32zLPiwKSY4axjmWoEvw7dsgBhfdDWKbs
> TH
> abgRnlxF0CbvNgoUi8d9BENCvZXkcTewIJdEpsV5oQ1w9wKjJX75XG/ggdrvrclh
> pGY1+Jglqv7ZKeTzGnHNaC0mVgMcJPK1mdr8lUrwn9M+Pmfi8Mk7nHy0XnvdkXa0
> NG4bt/9pWKc92zhVIHatu+61+oWu1b8VHOK/5i6+Ajl439efNMJZS3qYup5gReFF
> zVMyDAcPKEMNAZq/+gbNgkpeH3VARAsDTM6NhrDo4w3vrxkV/NL/TC7v5bDVVJ
> z0
> wE+6d+B2l3oKnHAqPXv5vpVLDg/0DeI8A620kIEsMRPdcvLm/oej1KQnDgtS5NMW
> 3flVKJ4vzWzzDe8TH4V4JEttj9BsbV+3eyI49Cu4WEKnaJAFBRz1pjGiyHs8hxap
> /9LhpimxDhWQ9X9ly5duLeF0hnIgFKDVqndAMIFMvAxeIl7rgX4OOUBnsUmONds
> p
> 5Frva8NdWwCHYnY+ocjZ1gPzePjIyVvYiPX5bLl3ATcpw3PrN4d6lBWnZKMMvcdS
> qH+9BUIdxVxh6haXf6Q6YD2QkTfaQs8c6357LIyvbED3z9X9yiFV4fHtSQCKogD1
> 0B1wQs3SHrsm6+QOd2NB
> =R959
> -----END PGP SIGNATURE-----