Subject: Re: [apps-discuss] appsdir early review of draft-ietf-cdni-redirection

From: ⌘ Matt Miller <[email-masked]>
Date: Tue, 14 Apr 2015 08:26:52 -0600

[ Resending to include apps-discuss ]

> I have been selected as the Applications Area Directorate early 
> reviewer for this draft (for background on appsdir, please see 
> http://trac.tools.ietf.org/area/app/trac/wiki/ 
> ApplicationsAreaDirectorate).
> 
> Please resolve these comments along with any other Last Call 
> comments you may receive. Please wait for direction from your 
> document shepherd or AD before posting a new version of the draft.
> 
> Document: draft-ietf-cdni-redirection Title: Request Routing 
> Redirection Interface for CDN Interconnection Reviewer: Matthew 
> Miller Review Date: 2015-04-13 IETF Last Call Date: N/A IESG 
> Telechat Date: N/A
> 
> 
> Summary: This draft has minor issues, and is ready for publication
>  as a Proposed Standard once these issues are resolved.
> 
> Major Issues:  None
> 
> Minor Issues:
> 
> * In Section 4.2., some of the language about the JSON encoding 
> correctness can be handled by conforming to I-JSON [RFC7493],
> which this draft already does in spirit.
> 
> * In Section 4.5.2., it's not clear why the keys "sc(location)", 
> "sc(cache-control)", and "sc(<headername>)" are different from the 
> other "sc-"-based keys; I would have expected them to be 
> "sc-location", "sc-cache-control", and "sc-<headername>".  This 
> deviates from the text in Section 4.5 that says "sc- is prefixed
> to keys for information to be passed by the Server (uCDN) to the 
> Client (User Agent)."  I suggest you either change the keys to be 
> "sc-<field>" instead of "sc(<field>)", or change the text in 
> Section 4.5 .
> 
> * In Section 4.7., it seems very odd that all of the error object 
> keys are optional.  Is the intent really that the following is 
> valid?
> 
> "error": {}
> 
> If it is, then an explanation of when and why that is valid would 
> be very helpful to me.  Otherwise, I suggest mandating at least 
> "error-code ".
> 
> * In Section 5., I would strongly consider referring to 
> draft-ietf-uta-tls-bcp (currently in the RFC Editor's Queue), and 
> incorporating its recommendations.  I think this draft
> accomplishes the basics, but I think you get a more complete
> picture with -tls-bcp.
> 
> Nits:
> 
> * The acronym CDN should be expanded in the Abstract.  Some might 
> also argue it should be expanded in the title, but not me.
> 
> * The terms list in Section 2. is editorially a bit awkward to me;
>  I expected there to be hanging indents.  In the XML, this can be 
> accomplished by wrapping the new terms' <t> with <list 
> style='hanging'> and placing the attribute hangtext equal to the 
> term for each terms' <t> (e.g., <t hangtext="Application Level 
> Redirection:">The act of using an application ...)
> 
> 
>

---

From: Ben Niven-Jenkins <[email-masked]>
Date: Tue, 21 Apr 2015 17:02:24 +0100

Hi Matt,

Thanks for the review, some responses inline.

> On 14 Apr 2015, at 15:26, ⌘ Matt Miller <[email-masked]> wrote:
> 
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
> 

I'm not sure it results in being able to remove much text from Section 4.2 because from a quick scan of RFC7493 the only overlap is both documents state the JSON object MUST NOT contain duplicate keys/names but I'm OK with adding a reference to I-JSON e.g. 

OLD:
The body of RI requests and responses is a JSON object [RFC7159] containing a dictionary of key:value pairs.

NEW:
The body of RI requests and responses is a JSON object [RFC7159] that MUST conform to [RFC7493] containing a dictionary of key:value pairs.

>> "sc-<field>" instead of "sc(<field>)", or change the text in 

The keys sc(location), sc(cache-control) and sc(<headername>) are different to the other sc- keys because their values encode the values of HTTP headers, whereas the values of the other sc- keys encode information about the HTTP request/response that are not sent in HTTP headers (method, version, etc).

[Actually sc(location) and sc(cache-control) aren't special but are specific instances of sc(<headername>) called out as they are likely to be of particular interest for the use case of redirection between CDNs.]

The thinking of putting the header name in () was to avoid possible clashes, e.g. if someone wanted to return a HTTP Header called "status" they wouldn't be able to do so because sc-status is already used for carrying the HTTP response's status code.

The simple fix would be to change sc(<headername>) to sc-(<headername>) so we avoid any potential clash and the format of the key is aligned with section 4.5 that says they start sc- (and do the same for the cs- keys too).

>> be very helpful to me.  Otherwise, I suggest mandating at least 

Good catch, will change so that error-code is mandatory.

>> accomplishes the basics, but I think you get a more complete

There are some discussions ongoing in the CDNI WG on referencing draft-ietf-uta-tls-bcp instead of articulating specific requirements in each of the CDNI specifications and we plan to align draft-ietf-cdni-redirection with the CDNI WG consensus for that text, which looks likely to end up being a reference to draft-ietf-uta-tls-bcp.

>> also argue it should be expanded in the title, but not me.

Thanks, will expand.

>> Redirection:">The act of using an application ...)

Thanks, will try this in the next revision.

Ben

>> 
>> 
>> 
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
> Comment: GPGTools - https://gpgtools.org
> 
> iQEcBAEBCgAGBQJVLSOsAAoJEDWi+S0W7cO1fZ8H/0Ew+qOZGyMz5JnBiFgajGWA
> JddcGDOKES3wSdO4rwnG0y+D9GRHz/3JYmhDalyo/8MvDSsxQ+Z8vDuN0AEk5dLV
> QOe/4DR4T10v9z5ZPcwQSEXY3dYSJoXc7Je0f7gw4Dhnp1croQ4o+6Qo8+W7pGwt
> 7jLzkeeKR8FJ9gPf5sQ0/zPBXf5Nhmgs9d/ZjbzuLVJOIpt77sr09dIwq8k30PaB
> O3765+dgH0jRjgaZuu7VZU429/TwDE/uaffCG8UcBDXjh6j/7zEgHwTDIwTj+5Ed
> 3onZ2bha/Q10vGUO7ooaj5jNNB2r1I/P9F4FSk8DMH7ioMP5KEI6e5Bml4z0/8U=
> =oeMr
> -----END PGP SIGNATURE-----
> 
> _______________________________________________
> apps-discuss mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/apps-discuss

---

From: Ben Niven-Jenkins <[email-masked]>
Date: Thu, 23 Apr 2015 16:10:03 +0100

Hi Matt,

Following up on my previous email, FYI I pushed out a -09 version of draft-ietf-cdni-redirection which I believe addresses all of your comments except the security considerations (which I will change once consensus is reached in the CDNI WG on appropriate text).

New draft here:
http://www.ietf.org/internet-drafts/draft-ietf-cdni-redirection-09.txt

Diff from -08 here:
http://www.ietf.org/rfcdiff?url2=draft-ietf-cdni-redirection-09

Ben

> On 21 Apr 2015, at 17:02, Ben Niven-Jenkins <[email-masked]> wrote:
> 
> Hi Matt,
>

---

From: ⌘ Matt Miller <[email-masked]>
Date: Thu, 23 Apr 2015 09:44:58 -0600

Thanks Ben.

All of your suggested changes below look good, and I'll review -09 as
my earliest chance.

- -- 
- - m&m

Matt Miller < [email-masked] >
Cisco Systems, Inc.

On 4/23/15 9:10 AM, Ben Niven-Jenkins wrote:
> Hi Matt,
> 
> Following up on my previous email, FYI I pushed out a -09 version
> of draft-ietf-cdni-redirection which I believe addresses all of
> your comments except the security considerations (which I will
> change once consensus is reached in the CDNI WG on appropriate
> text).
> 
> New draft here: 
>>>>> * In Section 4.2., some of the language about the JSON
>>>>> encoding correctness can be handled by conforming to I-JSON
>>>>> [RFC7493], which this draft already does in spirit.
>>> 
>>> I'm not sure it results in being able to remove much text from
>>> Section 4.2 because from a quick scan of RFC7493 the only
>>> overlap is both documents state the JSON object MUST NOT
>>> contain duplicate keys/names but I'm OK with adding a reference
>>> to I-JSON e.g.
>>> 
>>> OLD: The body of RI requests and responses is a JSON object
>>> [RFC7159] containing a dictionary of key:value pairs.
>>> 
>>> NEW: The body of RI requests and responses is a JSON object
>>> [RFC7159] that MUST conform to [RFC7493] containing a
>>> dictionary of key:value pairs.
>>> 
>>>>> * In Section 4.5.2., it's not clear why the keys
>>>>> "sc(location)", "sc(cache-control)", and "sc(<headername>)"
>>>>> are different from the other "sc-"-based keys; I would have
>>>>> expected them to be "sc-location", "sc-cache-control", and
>>>>> "sc-<headername>".  This deviates from the text in Section
>>>>> 4.5 that says "sc- is prefixed to keys for information to
>>>>> be passed by the Server (uCDN) to the Client (User Agent)."
>>>>> I suggest you either change the keys to be "sc-<field>"
>>>>> instead of "sc(<field>)", or change the text in Section 4.5
>>>>> .
>>> 
>>> The keys sc(location), sc(cache-control) and sc(<headername>)
>>> are different to the other sc- keys because their values encode
>>> the values of HTTP headers, whereas the values of the other sc-
>>> keys encode information about the HTTP request/response that
>>> are not sent in HTTP headers (method, version, etc).
>>> 
>>> [Actually sc(location) and sc(cache-control) aren't special but
>>> are specific instances of sc(<headername>) called out as they
>>> are likely to be of particular interest for the use case of
>>> redirection between CDNs.]
>>> 
>>> The thinking of putting the header name in () was to avoid
>>> possible clashes, e.g. if someone wanted to return a HTTP
>>> Header called "status" they wouldn't be able to do so because
>>> sc-status is already used for carrying the HTTP response's
>>> status code.
>>> 
>>> The simple fix would be to change sc(<headername>) to
>>> sc-(<headername>) so we avoid any potential clash and the
>>> format of the key is aligned with section 4.5 that says they
>>> start sc- (and do the same for the cs- keys too).
>>> 
>>>>> * In Section 4.7., it seems very odd that all of the error
>>>>> object keys are optional.  Is the intent really that the
>>>>> following is valid?
>>>>> 
>>>>> "error": {}
>>>>> 
>>>>> If it is, then an explanation of when and why that is valid
>>>>> would be very helpful to me.  Otherwise, I suggest
>>>>> mandating at least "error-code ".
>>> 
>>>>> draft-ietf-uta-tls-bcp (currently in the RFC Editor's
>>>>> Queue), and incorporating its recommendations.  I think
>>>>> this draft accomplishes the basics, but I think you get a
>>>>> more complete picture with -tls-bcp.
>>> 
>>> There are some discussions ongoing in the CDNI WG on
>>> referencing draft-ietf-uta-tls-bcp instead of articulating
>>> specific requirements in each of the CDNI specifications and we
>>> plan to align draft-ietf-cdni-redirection with the CDNI WG
>>> consensus for that text, which looks likely to end up being a
>>> reference to draft-ietf-uta-tls-bcp.
>>> 
>>>>> 
>>>>> Nits:
>>>>> 
>>>>> * The acronym CDN should be expanded in the Abstract.  Some
>>>>> might also argue it should be expanded in the title, but
>>>>> not me.
>>> 
>>> Thanks, will expand.
>>> 
>>>>> * The terms list in Section 2. is editorially a bit awkward
>>>>> to me; I expected there to be hanging indents.  In the XML,
>>>>> this can be accomplished by wrapping the new terms' <t>
>>>>> with <list style='hanging'> and placing the attribute
>>>>> hangtext equal to the term for each terms' <t> (e.g., <t
>>>>> hangtext="Application Level Redirection:">The act of using
>>>>> an application ...)
>>> 
>>> _______________________________________________ apps-discuss
>>> mailing list [email-masked]