Re: [Dime] AD review draft-ietf-dime-app-design-guide

[Benoit Claise <[e-mail]>]

Hi Lionel,
>
> Hi Benoit,
>
> Thank you for the deep review.
>
> Please find my feedback below.
>
> Regards,
>
> Lionel
>
> *De :*DiME [mailto:[e-mail]] *De la part de* Benoit Claise
> *Envoyé :* mercredi 2 avril 2014 11:04
> *À :* [e-mail]
> *Cc :* dime mailing list
> *Objet :* [Dime] AD review draft-ietf-dime-app-design-guide
>
> Dear authors,
>
> Sorry for dropping the ball on this one.
> If any of the points was already discussed/addressed by the WG, feel 
> free to let me know.
>
>
>
> - When I read the document, it looked to me as a BCP.
>
> BCP definition from RFC 2026:
>
>     5.  BEST CURRENT PRACTICE (BCP) RFCs
>
>       
>
>         The BCP subseries of the RFC series is designed to be a way to
>
>         standardize practices and the results of community deliberations.
>
> Interestingly, the charter mentions:
>
> May 2012, Submit 'Diameter Application Design Guidelines' to the IESG 
> for consideration as a BCP document
>
> */[LM] discussed in another email thread./*
>
>
> If you go to BCP, don't forget to update the abstract, and the writeup.
> Also, BCPs usually use the RFC2119 keywords (ex: RFC 7013)
> Example:
> OLD:
>
> Diameter
> protocol designers are then strongly advised to carefully consider
>
> NEW:
>
>     Diameter
>     protocol designers SHOULD NOT consider
>   
>
> OLD:
>
>     Instead of using
>     an Enumerated AVP for a Boolean flag, protocol designers are again
>     encouraged to use AVPs of type Unsigned32 or Unsigned64 AVP in which
>     the data field would be defined as
>
>
> NEW:
>
>     Instead of using
>     an Enumerated AVP for a Boolean flag, protocol designers SHOULD
>     use AVPs of type Unsigned32 or Unsigned64 AVP in which
>     the data field would be defined as
>
> Finally, with a BCP, RFC 6733 could be a normative reference, which I 
> believe it should.
>
>
> - Editorial
> Please don't use "we" in RFCs
>
> */[LM] Should I say "I"? /**/J/*
>
[BC] That's one of those rules I received from my previous ADs, and that 
I've been applying blindly
In fact, I can't find it at 
http://www.rfc-editor.org/rfc-style-guide/rfc-style
Copying Heather, who might be able to shed some light.
>
> *//*
>
>
>
> -
> Section 3
>
>    Major Extension:  Enhancing an application that requires the
>        definition of a new Diameter application.
>   
>        Typical examples would be the creation of a new command for
>        providing functionality not supported by existing applications or
>        the definition of a new AVP with the M-bit set to be carried in an
>        existing command.  For such extension, a significant specification
>        effort is required and a careful approach is recommended.
>
> Do you want to mention that Major Extension have backward 
> compatibility issue, as opposed to the minor one?
>
> */[LM] Yes. I'm assuming that you would prefer to say it explicitly./*
>
[BC] Yes.
>
> *//*
>
>
>
> - Editorial
>
>        Typical examples would be the creation of a new command for
>        providing functionality not supported by existing applications or
>        the definition of a new AVP with the M-bit set to be carried in an
>        existing command.  For such extension, a significant specification
>        effort is required and a careful approach is recommended.
>
> NEW:
>
>        Typical examples would be the creation of a new command for
>        providing functionality not supported by existing applications or
>        the definition of a new mandatory AVP set to be carried in an
>        existing command.  For such extension, a significant specification
>        effort is required and a careful approach is recommended.
>
> Justification:
>
>          * Minor extension speaks about "optional"
>          * The M-bit is explained in 4.3.1
> */[LM] I would say that "mandatory" and "M-bit" set are the same and you need to refer to RFC6733 to understand the meaning anyway. But I would be prefer to keep "M-bit set" to avoid the common ambiguity with "required AVP" that indicates the mandatory presence of an AVP. I can add an explicit reference to section 4.1. of RFC6733./*
[BC] Ok
>
>
> - Section 3
> I see Minor Extension versus Major Extension, and I tried to match the 
> following classifications to Minor or Major
>
>     1.  Addition of new functionality to an existing Diameter application
>         without defining a new application.
>   
>     2.  Addition of new functionality to an existing Diameter application
>         that requires the definition of a new application.
>   
>     3.  The definition of an entirely new Diameter application to offer
>         functionality not supported by existing applications.
>   
>     4.  The definition of a new generic functionality that can be reused
>         across different applications.
>
> 2 and 3 are Major
>
> */[LM] right/*
>

> *//*
>
>
> For 1, I thought about Minor, but the following sentence "or the 
> definition of a new AVP with the M-bit set to be carried in an 
> existing command." in the Major paragraph confuses me.
>
> */[LM] (1) is "Minor" as no new application is required. If the new functionality would have been supported by an AVP with the M-bit set, it would have caused the creation of a new application (cf. section 4.3.1: "/*A mandatory AVP cannot be added to an existing command without defining a new Diameter application*/"/*
>
>
> I guess that 4 is Major, but it's not mentioned.
>
> */[LM] can be both as described in section 6./*
>
>
> Can you please better explain the mapping between the 4 categories and 
> Minor/Major extensions
>
> */[LM] the whole document is about clarifying these points. Each point 
> is further described in the section 4, 5 and 6. /*
>
>
> Alternatively, or maybe on top of that, explain which of 4.X and 5.X 
> are Minor/Major extensions would be beneficial.
>
> */[LM] I will try to clarify when required by highlighting "Minor/Major"/*
>
[BC] perfect.
>
> *//*
>
>
>
> - Section 3
> I don't understand what your message is with:
>
>     We would also like to remind that the definition of a new Diameter
>     application and the definition of a new command should be something
>     to avoid as much as possible.  In the past, there has been some
>     reluctance to define new commands and new applications.  With the
>     modified extensibility rules provided by [RFC6733  <http://tools.ietf.org/html/rfc6733>], registering new
>     commands and new applications does not lead to additional overhead
>     for the specification author in terms of standardization process.
>     Registering new functionality (new commands, new AVPs, new
>     applications, etc.) with IANA remains important to avoid namespace
>     collisions, which will likely lead to deployment problems.
>
> "should be something to avoid as much as possible", is this valid today?
> Because the next sentence speaks about the past, then the next 
> sentence seems like it's fixed now with 6733.
>
> */[LM] Some inconsistency due to the modification of the IANA 
> allocation modalities. I propose to:/*
>
> -*/ remove the first sentence./*
>
> -*/Clarify that "in the past" refers to RFC3588/*
>
> -*/Remove the last sentence as it is somehow clarify in the section 7./*
>
> -*/Add a sentence that the general guideline "try to reuse as much as 
> possible" still applies./*
>
> *//*
>
[BC] I'll review the proposed text.
>
>
> - Editorial:
>
>     With the
>     modified extensibility rules provided by [RFC6733  <http://tools.ietf.org/html/rfc6733>], registering new
>     commands and new applications does not lead to additional overhead
>     for the specification author in terms of standardization process.
>     Registering new functionality (new commands, new AVPs, new
>     applications, etc.)
>
> "etc.": What does it refer to? new AVP value is the only one I can 
> think of.
> Worth removing "etc." and specifying the full list?
>
> */[LM] removed./*
>
>
> - Application and command
>
>     Major Extension:  Enhancing an application that requires the
>        definition of anew Diameter application.
>   
>        Typical examples would be the creation of anew command  for
>        providing functionality not supported by existing applications or
>        the definition of a new AVP with the M-bit set to be carried in an
>        existing command.  For such extension, a significant specification
>        effort is required and a careful approach is recommended.
>
>
>       4.1
>       <http://tools.ietf.org/html/draft-ietf-dime-app-design-guide-21#section-4.1>. 
>       Adding a New Command
>
>     Adding a new command is considered as a major extension and requires
>     a new Diameter application to be defined.
>
> I'm not clear on the application/command boundary.
> Why do we need a new application for a new command?
>
> */[LM] using an unknown command in an existing application will cause 
> a protocol error as per the RFC 6733./*
>
> DIAMETER_COMMAND_UNSUPPORTED 3001
>
>       This error code is used when a Diameter entity receives a message
>
>       with a Command Code that it does not support.
>
> */Therefore non-supporting nodes will never be able to process the 
> request./*
>
>
> Why can't I add a command to an existing application?
> There are commands that are for all applications/independent of the 
> application, no?
>     CER/CEA (Capabilities-Exchange-Request)
>
> */[LM] CER/CEA are part of an application, the application "0" i.e. 
> the base protocol. The requirement is that all diameter peers support 
> AT LEAST the base protocol application./*
>
>
> This contradicts:
>
>     Before adding or_importing_a command, application designers
>     should consider the following ...
> */[LM] here, we are in the section "Adding a new command to an existing application" and it is clearly said that it is a major extension that will cause the creation of a new application./*
>   
> This also appears to contradict, in section 6
>     Generic Diameter extensions are AVPs, commands or applications that
>     are designed_to support other Diameter applications_.
> */[LM] not so. Any new application can reuse an existing command. For instance, Session Termination commands are reused in every session-related applications./*
>   
>
> My issue comes from the fact that there are no "application" and 
> "command" definitions.
> Draft says:
>
>
>     2
>     <http://tools.ietf.org/html/draft-ietf-dime-app-design-guide-21#section-2>.
>     Terminology
>
>     This document reuses the terminology defined in [RFC6733]  <http://tools.ietf.org/html/rfc6733>
>
> However, RFC 6733 terminology doesn't contain the application and 
> command definitions.
> Talking to Jouni, I now understand that a command is always specified 
> within the context of an application.
> This should be clarified.
>
> */[LM] ok/*
>
>
>
> Also, from section 5.2:
>
>     As a general recommendation, commands should not be defined from
>     scratch.  It is instead recommend to re-use an existing command
>     offering similar functionality and use it as a starting point.
>
> Based on my latest understanding (a command is always specified within 
> the context of an application), the only justification for the above 
> text is code reuse, right. Please mention it.
>
> */[LM] it refers to the text in the section 3./*
>
>    It will reduce the time to finalize
>
> specification writing, and it will lead to a smaller implementation
>
>    effort as well as reduce the need for testing.  In general, it is
>
>    clever to avoid duplicate effort when possible.
>
> */Will be clarified./*
>
[BC] thanks. I will review all this command and application text with 
the new version.
>
> *//*
>
>
>
> - Editorial
>
>     Adding a new command is considered as a major extension and requires
>     a new Diameter application to be defined.  Adding a new command to an
>     application means ...
>
> A new command addition is always to an application, right?
> If yes, why do you make the distinction between the sentences above
>
> */[LM] It is about adding a command to an EXISTING application. The 
> first sentence will say "/*Adding a new command to an existing 
> application is considered*/" and "to an application" will be removed 
> from the second sentence./*
>
>
>
> - Application version?
>
> An exception might be if the
>     intent of the deletion is to create a newer version of the same
>     application that is somehow simpler than the previous version.
>
> ...
>
>     o  Would the new AVP be used to differentiate between old and new
>        versions of the same application whereby the two versions are not
>        backward compatible?
>
> Readers might be understanding that diameter applications having a 
> version field.
> This is not the case. Please rephrase.
>
> */[LM] I think we can speak about new version of the specification and 
> variant of the application/*
>
>
> Also, mention that a new version of an application is a new application.
> I guess it needs to be mentioned in 7.d
>
> */[LM] OK for the clarification but not in 7.d as this section is only 
> about interaction with IANA./*
>
[BC] Sure
>
>
> - Section 4.3.2
> For the reader convenience, I would mention the convention behind { 
> AVP } and [ AVP ], or at least give a reference.
>
> */[LM] I would clarify that "/*a required AVP (an AVP indicated as {AVP} in the command CCF)*/" and"/*an optional AVP (an AVP indicated as [AVP] in the command CCF)*/"/*
>
>
>
> - Section 5.3
> OLD:
>
>     Some existing
>     specifications do not adhere to this rule for historical reasons.
>     However, this guidance should be followed to avoid routing problems.
>   
> NEW:
>     Some existing
>     specifications do not adhere to this rule for historical reasons.
>     However, this guidance should be followed for new applications to avoid routing problems.
> */[LM] ok/*
>
>
> In the same section, why "In general" in the next sentence? It 
> contradicts with "must use"
>
> In general, when a new application has been allocated with a new
>     Application Id and it also reuses existing commands with or without
>     modifications, it must use the newly allocated Application Id in the
>     header and in all relevant Application Id AVPs (Auth-Application-Id
>     or Acct-Application-Id) present in the commands message body.
> */[LM]  "In general" must be removed and "must use" will be "should use"./*
>
>
> - Editorial, section 5.5
>
> OLD:
>     Based on these considerations, protocol designers should carefully
>     appraise whether the application currently defined relies on it's own
>     session management concept or whether
>   
> NEW:
>     Based on these considerations, protocol designers should carefully
>     appraise whether the application currently defined relies on its own
>     session management concept or whether
> */[LM] ok/*
>
>
> - Editorial in section 5.7
> OLD:
>
> Destination- Realm
>   
> NEW:
> Destination-Realm
> */[LM] ok/*
>
>
>
> - The section 5.8 should mention RFC4005bis
>
> */[LM] already covered by a previous comment /**/J/**//*
>
>
>
> - Section 5.9
>
>   Applications that do not understand these AVPs can discard
>     them upon receipt.
>
> Generic comment: Each time there is a sentence like this one above, we 
> should mention RFC 6733 as the reference.
> This document is not an extension/deviation to RFC 6733.
>
> */[LM] ok/*
>
>
>
> - Do you have a good reason to add a reference to a work-in-progress 
> that didn't progress since 2001?
>
>     [I-D.calhoun-diameter-res-mgmt]
>                Calhoun, P., "Diameter Resource Management Extensions",
>                draft-calhoun-diameter-res-mgmt-08.txt  <http://tools.ietf.org/html/draft-calhoun-diameter-res-mgmt-08.txt>  (work in progress),
>                March 2001.
> */[LM] I think that it was as illustration of general-purpose extension based on new commands. Other existing examples are only based on introduction of new AVP. But I agree that it is weird to make reference to expired drafts/**/J/**/   I would rather refer to Realm-Based Redirection (RFC 7075) and Diameter Priority AVP (RFC6735)/*
>
>
> - Editorial (section 6)
> I can't parse the following sentence:
>
>   Backward Compatibility:
>   
>        With the design of generic extensions an protocol designer has to
>        consider with potential concerns about how existing applications
>        deal with the new extension they do not understand.
> */[LM] what about:/*
> */  /*
>        When defining generic extensionsdesigned to be supported by existing
>        Diameter applications, protocol designers have to consider the
>        potential impacts of the introduction of the new extension on the behavior
>        of node that would not be yet upgraded to support/understand this new extension.
>   
[BC] Fine
>
>
> - Contributors:
> If Victor and Hannes are authors, then they shouldn't be in the 
> contributors list.
>
> */[LM] this list gives the members of the design team, not the 
> contributors./*
>
[BC]
http://www.rfc-editor.org/policy.html#policy.auth
     When the RFC Editor refers to "contributors", we mean people, other 
than the authors, who also contributed significantly to the RFC

> *//*
>
>
> Btw, I don't see an affiliation for Victor in the document header. I 
> believe the common practice is to write down "consultant"
>
> */[LM] I will try to contact him to see what he would like to see 
> indicated as affiliation./*
>
>
>
> Did the WG ever considered the following questions:
> - Any naming convention for new applications?
>
> */[LM] no convention so far./*
>
[BC] Ok, then.
As an example, this was covered for IPFIX IE in RFC 7013
>
> *//*
>
>
> - When it is not a good idea to define diameter applications? Let me 
> make something up: to push syslog message
>
> */[LM] let me two days and I will define a nice application for that 
> /**/J/**/I cannot see real key criteria to formally say "defining a 
> Diameter application for that purpose is a bad idea". If your example 
> was for "to reinvent the wheel", it seems that this is only a 
> criterion for WG chairs and Ads, but not anymore for the industry. /*
>
[BC]  Ok, then.
I just don't want that, in 6 months from now, the WG comes up with a 
draft such as draft-shore-icmp-aup-12 
<https://datatracker.ietf.org/doc/draft-shore-icmp-aup/>, specifically 
for DIME.

Regards, Benoit
>
>
> Regards, Benoit
>
>
>
>
>
>
>
> _________________________________________________________________________________________________________________________
>
> Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
> pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
> a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
> Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.
>
> This message and its attachments may contain confidential or privileged information that may be protected by law;
> they should not be distributed, used or copied without authorisation.
> If you have received this email in error, please notify the sender and delete this message and its attachments.
> As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
> Thank you.