Diameter Applications Design Guidelines
draft-ietf-dime-app-design-guide-28

IANA has reviewed section 7, so I'm marking the DISCUSS as resolved.   All other comments still apply, of course.

In 4.2:
   It is unusual to delete an existing command from an application for
   the sake of deleting it or the functionality it represents.  This
   normally indicates of a flawed design.  An exception might be if the
   intent of the deletion is to create a newer variance of the same
   application that is somehow simpler than the application initially
   specified.

The second sentence seems to be making the same point as the third sentence: you are deleting a command because the original design is flawed.   By stating it this way you seem to be weakening the point.   Is that what was intended, or is the second sentence referring to the _new_ design?   If the latter, some wordsmithing here might improve the document.

In 4.3.1:

   o  Adding one or more optional AVPs and indicating (usually within
      descriptive text for the command) that at least one of them has to
      be present in the command.  This essentially circumventing the
      ABNF and is equivalent to adding a mandatory AVP to the command.

Isn't this a MUST, not a SHOULD?  In general, if the possibilities enumerated in this list are definitely going to cause interop problems, they should be MUSTs; if they are likely to cause interop problems, you should specify a test that indicates when violating the SHOULD is okay.   I don't think the test will be precise, but a little bit clearer guidance might be beneficial.

In 4.4.1, it might be helpful to talk about the case where a new application makes an AVP mandatory that is not mandatory in existing applications.   Suppose an AVP is received without the M bit set when it's mandatory for that application.   Is that an error?   Should the recipient reject the communication? 

In 5.2:
   As a general recommendation, commands SHOULD not be defined from

SHOULD NOT the NOT be capitalized, to be consistent with the SHOULD? :) I see that this occurs again several times in the document. Ordinarily SHOULD and NOT are capitalized consistently, whether it's "should not" or "SHOULD NOT" and not "SHOULD not." But this is a fairly minor nit.

   Defining a command with most of the AVPs indicated as
   optional MUST NOT be seen as a sub-optimal design introducing too
   much flexibility in the protocol.

It's a little weird to use normative language to tell people how to think about something.   The use of normative language in this document in general is a bit challenging, but I think appropriate because you're specifying best practices for achieving interoperability.   But in this case I think you should not use normative language.   I'd word it this way:

   Defining a command with most of the AVPs indicated as
   optional is good design in many cases, despite the flexibility
   it introduces in the protocol.

Or just restate it as you did, but as a fact rather than an admonition about how to think.

And then:

   The protocol designers SHOULD only
   clearly state the condition of presence of these AVPs and properly
   define the corresponding behaviour of the Diameter nodes when these
   AVPs are absent from the command.

How about:

   Protocol designers SHOULD
   clearly state the reasons why these AVPs might or
   might not be present and properly define the
   corresponding behavior of the Diameter nodes
   when these AVPs are absent from the command.

And:
      NOTE:  As a hint for protocol designers, it is not sufficient to just
      look at the command's CCF syntax specification.  It is also
      necessary to carefully read through the accompanying text in the
      specification.

This looks like a hint for implementors, not protocol designers.   I am confused.   Do you mean that protocol designers should, when defining the spec, include a recommendation that implementors read the accompanying text as well as the CCF specification?

In 5.5:
   Based on these considerations, protocol designers SHOULD carefully
   appraise whether the application currently defined relies on its own
   session management concept or whether the Session-Id defined in the
   Diameter base protocol would be used for correlation of messages
   related to the same session.  If not, the protocol designers MAY
   decide to define application commands without the Session-Id AVP.

This is stated in an overly detailed way, and "if not" is confusing, because the preceding statement contains two possibilities, neither of which is obviously negative.   It's clear on reflection what is meant, but it might be better stated something like this:

   Based on these considerations, protocol designers SHOULD carefully
   consider whether the application currently defined relies on its own
   session management concept.  If not, the protocol designers MAY
   decide to define application commands without the Session-Id AVP,
   and rely on the Session-Id defined in the
   Diameter base protocol for correlation of messages
   related to the same session.

Also:
   If any session management concept is supported by the application, the
   application documentation MUST clearly specify how the session is
   handled between client and server (as possibly Diameter agents in the
   path).

Was that supposed to be "and possibly" rather than "as possibly"?

In 5.6:
   If such extension is
   foreseen or cannot be avoided, it is RECOMMENED to rather define AVPs
   of type Unsigned32 or Unsigned64 in which the data field would
   contain an address space representing "values" that would have the
   same use of Enumerated values.

It is not obvious to me why this is so.   It seems as if using an unsigned integer with some set of known values in application A, and then adding some new values to the same AVP in application B, is no different than defining an enumeration in application A with some set of possible values, and extending the enumeration in application B with some additional values.

I am sure that there is a reason why you have made this distinction, and I could take a guess as to what the reason is, but my point is that you should really just state what the reason is clearly in this section, rather than simply stating your conclusion, because as written the section is really puzzling to me, a reader who is not steeped in the traditions of Diameter.

In section 5.7:
   Whatever the criteria used to establish the routing path of the
   request, the routing of the answer MUST follow the reverse path of
   the request, as described in [RFC6733], with the answer being sent to
   the source of the received request, using transaction states and hop-
   by-hop identifier matching.  In particular, this ensures that the
   Diameter Relay or Proxy agents in the request routing path will be
   able to release the transaction state upon receipt of the
   corresponding answer, avoiding unnecessary failover.  Application
   designers SHOULD NOT modify the answer-routing principles described
   in [RFC6733] when defining a new application.

This seems like something that could, if ignored, result in some bad behavior.   It also seems like something that's not clearly enforced by a mechanism that you've pointed out here.   So it would be good if the risk of abuse by clients responding to such messages could be discussed either here or in the security considerations section, and if there is some recommended mitigation strategy, that it be mentioned as well.   Possibly there is no risk, but that doesn't seem likely since if that were the case, this text would seem unnecessary.

Section 5.11 appears to be speaking to implementors.   The advice it gives is good, but is there an expectation that implementors will read this document?

In 6:
   In the case where the optional
   AVPs can be carried by any application, it SHOULD be sufficient to
   specify such a use case and perhaps provide specific examples of
   applications using them.

I can't see how SHOULD could be normative here, so perhaps it shouldn't be uppercase?

This is a great document.   Thanks for doing it!