Subject: [core] Review on draft-ietf-core-coap-pubsub-02

From: Jim Schaad <[email-masked]>
Date: Mon, 10 Jul 2017 11:57:00 -0700

Status:  This document is not yet ready for a WGLC in my opinion. 

I have copied forward some comments from past reviews where I have not seem
them addressed or do not believe that they are sufficiently  dealt with.

* Section 3.4 - s/are hosted at the broker and all clients using the broker
share/are hosted by a broker and all clients using that broker share/ - a)
you can have multiple brokers and b) the name spaces in that case are going
to be distinct and potentially overlapping in name w/different values.

* Section 3.4 - "zero or more stored representations with a content-format"
I think this is supposed to be  "zero or more values - one content-format".
But the first time I read this, I read this as" zero or more representations
- each representation has a content format - may be zero or one (?) value".
I always worry about what is "current" not what is "history" and assume that
observe is correctly implemented.  Note this does go back to the question of
"is this a queue of values" that Hannes has raised in the past.

* Section 4.2 - cut and paste error for topic and q as template variables.
I believe that q should be removed from the template as it makes no sense.

* Section 4.2 - Is the following legal for payload of a create
"<mainTopic/subTopic>;ct=50"  - this is a legal link but may not be legal
for creation of a topic.  

* Section 4.2 - Is the following legal for the payload of a create
"<coap://localhost/ps/maintopic/subtopic>;ct=50" - The text current says the
target of the link is a "URI formatted string" rather than a fragment.

* Section 4.2 - I cannot tell if the requirement for returning 4.04 (not
found) is new in this document or should be part of RFC 6690 or not.  I
cannot seem to find this requirement in that document.  Are you sure you
want to do this rather than just return an empty content?  Should there be
an errata on RFC 6690?

* Section 4.2 - There is an implicit statement in section 4.3 that a single
content type is expected.  However, there is no explicit statement here that
a request with multiple content types is forbidden.  Is there really a
requirement that a single content type be required when creating a topic?
What happens if no 'ct' is specified?

* Section 4.3 - Is a server supposed to accept a publish operation to a
non-existent location with a POST method? 

* Section 4.3 - Is there any reason to accept publish via POST and not PUT?
Or the other way around?
 
* Section 4.3 - If no Max-Age is on the publish request, should the default
value be used or should an infinite value (i.e. absent) be used.  If a
Max-Age was supplied at the time the subject was created, does that change
things?  If a publish is done with a Max-Age greater than that provided at
creation, what should happen.  Which wins?

* Section 4.3 - Copy and paste error on the topic and q template variables.
Does 'q' even make sense here?

* Section 4.3 - Text dealing with propagating publish events up the tree to
keep nodes alive is missing.

* Section 4.4 - Copy and paste error on the topic and q template variables.
Does 'q' even make sense here?

* Section 4.4 - Is it permitted that a broker remove a subscription due to
lack of ACK for a CON w/o sending the "unsubscribed" message?  The document
does not seem to allow that.

* Section 4.5 - Copy and paste error on the topic and q template variables.
Does 'q' even make sense here?

* Section 4.6 - Copy and paste error on the topic and q template variables.
Does 'q' even make sense here?

* Section 4.7 - Is remove recursive?   

* Section 4.7 - What would the correct content be to send an unsubscribe
notification on a remove operation?  '2.07 no content' or '2.02' deleted or
'4.04' not found?

* Section 8 - If enforcing access policies is of importance, how are they
set?

* Section 8 - It may be that '4.01 Unauthorized' is a better response than
'4.04 Not Found' in some cases.  Doing this does not allow for probing of
what topics exist when one does not have access to the directory of topics.

* Consider the following scenario
  - Create node foo - ct=40, Max-Age=10
  - Create node foo/bar - ct=0, Max-Age=20
  - Publish to foo/bar @ time=5
  When you get to time=15, should the node foo be deleted or does the
max-age of foo/bar change that answer?

* Currently an application can use CON to ensure that a server will get a
message in the long run.  The fact that this is no doable here should be
briefly discussed as an client cannot know that a server has or has not
received a message.

Minor:

* Section 3.4 -  s$"EP-33543/sen$"/EP-35543/sen$ - either that or remove the
leading slash for sensors

Philosophy problem:

I am not happy with the way that 2119 language is sprinkled throughout this
document.  That is not a statement that you need to change it, but it is a
request that you revisit and look at what is there.  
* MUST and SHOULD statements need to be able to have some type of test
written from them.  If you cannot write a test, then using the key words is
probably incorrect.
* SHOULD and MAY statements ought to have some type of language about when
the action under consideration would not be done.  
* MAY statements are almost always a waste of time and should be written in
a different manner. 
    *  Consider the first sentence in section 4.3.  Is there a reason why
this is not changed to "This section is optional to implement." and leave it
at that?  
    *  Consider the second sentence in section 4.3.  It implies that there
is a third method that the client can use to publish, what is that method?
I don't currently know of one.  This saying "A client uses the POST or PUT
method to publish ..." seems to be an adequate statement w/o the MAY.  
* Once upon a time, there was a requirement to extract and test all of the
MUSTs in a document and a strong suggestion to test all of the SHOULDs in a
document.  Too many make things hard to see if you have adequate coverage.
Too few and you might not have interop working correctly.  In many cases, a
MUST can be expressed w/o the key work.  Consider the 2.04 sentence in
section 4.3 paragraph 1 - It can be written as "The broker returns a "2.04
Changed" response if the publish request is accepted".  This has the same
information as the current statement but w/o the MUST.  The two following
sentences should probably also be a MUST type statement so that a client
knows what is going on rather than returning something strange.  (That can
be countered in the case of '4.04 Not Found' to maybe be '4.01 Unauthorized'
as a reasonable response to prevent knowledge of what topics exist.)

Jim

---

From: Michael Koster <[email-masked]>
Date: Thu, 20 Jul 2017 16:07:38 -0700

Hi Jim,

Thank you for taking the time to evaluate this draft. We have discussed each point and have included the discussion and resolution inline in this response.

> On Jul 10, 2017, at 11:57 AM, Jim Schaad <[email-masked]> wrote:
> 

Yes, updated
> "is this a queue of values" that Hannes has raised in the past.
We think stored representations is correct, as a broker only stores representations and is not aware of values.
We are restricting it to most recently published representation, and should take care that this is clear.

For queueing and reliable notification, we consider a future extension based on subscriber-managed queues. 
See github issues: 
https://github.com/core-wg/pubsub/issues/5 <https://github.com/core-wg/pubsub/issues/5>

> I believe that q should be removed from the template as it makes no sense.
Yes, corrected.

> "<mainTopic/subTopic>;ct=50"  - this is a legal link but may not be legal
Yes, we will clarify what is and is not legal for create. Probably only one topic tree level at a time.

> target of the link is a "URI formatted string" rather than a fragment.
Same as above.WIll clarify in the next update.

> want to do this rather than just return an empty content?  Should there be
RFC6690 should not specify how REST APIs are expected to expose links. It should focus on the format and just provide examples. 4.04 is returned when the topic has been removed. Subscribe to non-existant topics may be covered in the subscriber queue extension described above.

> What happens if no 'ct' is specified?
Candidate text:
A topic creator includes one or more content format option values in the create payload. If the broker does not support all of the indicated formats for both publish and subscribe, it (SHALL) returns an error response.

If a broker allows a topic to be created with more than one content-format, it SHALL support an arbitrary mix of the supported content formats in publish and subscribe operations.

There is no default content format. If no ct is specified, the broker SHALL reject the operation with an error code 4.15 "unsupported content format" 

> non-existent location with a POST method? 
Create-on-publish must use PUT. The link is constructed from the request parameters. WIll clarify in the next update.

> * Section 4.3 - Is there any reason to accept publish via POST and not PUT?
PUT is required for create-on-publish

POST is prefereble when the topic already exists, but PUT may be used also. 

Generically, PUT is defined as replace the stored representation with the supplied representation, may create if the server supports (i.e. a PUT to a non-existent location may return 2.01 Created or 4.04 Not Found)

> creation, what should happen.  Which wins?
More explanatory text is perhaps needed. There are two Max-Age contexts, one is for the topic and one is for the value.

If no Max-Age is supplied, the topic should be permanent, modulo the server's abiity to store topics.

Max-Age on publish should default to permenent also.

> keep nodes alive is missing.
Yes, this is the intended behavior and will be described in the next update.

> Does 'q' even make sense here?
Yes, corrected in the next update.

> does not seem to allow that.
If a broker removes a topic, it is allowed to do so without informing the subscribers. We use a SHOULD statement to indicate that. 

If a client fails to ACK an observe response when the broker uses CON, the broker should not try to send a final message. We will clarify this. RFC7641 section 3.5 doesn't seem to explain this either:

   An acknowledgement message signals to the server that the client is
   alive and interested in receiving further notifications; if the
   server does not receive an acknowledgement in reply to a confirmable
   notification, it will assume that the client is no longer interested
   and will eventually remove the associated entry from the list of
   observers (Section 4.5 <https://tools.ietf.org/html/rfc7641#section-4.5>).

> Does 'q' even make sense here?
Yes, will correct in the next update.

> Does 'q' even make sense here?
Yes. Will correct.

> * Section 4.7 - Is remove recursive?   
Remove is inclusive of sub-topics, or recursive from the bottom of the tree. WIll add text to clarify.

> notification on a remove operation?  '2.07 no content' or '2.02' deleted or
2.02 Deleted would be the response to the Deleteing client, 4.04 woud be sent to observing clients, as per standard CoAP Observe. Will make sure this is clear in the next update.

> * Section 8 - If enforcing access policies is of importance, how are they
The reference is toward an ACL type system that may be out of the scope of this draft. More could be said about what to do and how to do it. Will address this after some more discussion.

> what topics exist when one does not have access to the directory of topics.
Good point. This may be added to security considerations after discussion. It would be better to have explicit ACLs that deny access to the resources and requre returning 4.01 in these cases.
> max-age of foo/bar change that answer?
foo expires and inclusively removes foo/bar at t=15

> briefly discussed as an client cannot know that a server has or has not
Yes, end-to-end protocol does not exist in pubsub. We have stayed away from describing transfer layer operations in this draft, but it seems like a good idea to point out the considerations in an informative way here as in the issue above about observe termination. 
> leading slash for sensors
Yes, appears to be a typo.
> as a reasonable response to prevent knowledge of what topics exist.)
Thanks for this advice. Will discuss and improve the normative language. Thinking about test cases is indeed a good way to crisp up the normative stuff, as it makes it painful to try to test things that don't need to be or shouldn't be prescriptive.