ietf-wg-ppm / draft-ietf-ppm-dap

This document describes the Distributed Aggregation Protocol (DAP) being developed by the PPM working group at IETF.
Other
46 stars 22 forks source link

Alignment with RFC 7807 Problem Details #209

Closed divergentdave closed 1 year ago

divergentdave commented 2 years ago

Some of the requirements and recommendations in section 3.1 conflict with the spirit of RFC 7807, which it references.

There is presently a requirement that:

The "instance" value MUST be the endpoint to which the request was targeted.

The "instance" key is defined as one of the standard members in RFC 7807, and it says:

"instance" (string) - A URI reference that identifies the specific occurrence of the problem. It may or may not yield further information if dereferenced.

As a consequence of the current text, PPM aggregators would always provide the same URI in the "instance" field. I would recommend that we remove the MUST requirement here. Alternately, we could define a new extension member (say, "endpoint") and put the aggregator's endpoint in that member, if we still wish to have it as part of the problem details. (For what it's worth, RFC 7807 allows the "type" and "instance" members to be relative URIs, so there is already a baked-in assumption that problem detail consumers retain the request URL associated with a JSON document)

Second, there's a SHOULD recommendation that the "detail" field be populated, and no mention of the "title" field. These are both standard members, defined as follows:

"title" (string) - A short, human-readable summary of the problem type. It SHOULD NOT change from occurrence to occurrence of the problem, except for purposes of localization (e.g., using proactive content negotiation; see [RFC7231], Section 3.4). "detail" (string) - A human-readable explanation specific to this occurrence of the problem.

I think we should add a recommendation that, when one of the error types from Table 1 is used, the accompanying description from the table (or a localization thereof) should be provided in the "title" member. The "detail" member could then provide more particularized information, for example, a message mentioning the old and new configuration IDs involved in an urn:ietf:params:ppm:error:outdatedConfig error.

Third, there is currently the following requirement for a PPM-specific extension member:

The problem document MUST also include a "taskid" member which contains the associated PPM task ID (this value is always known, see {{task-configuration}}).

Assuming that an aggregator services multiple PPM tasks at one HTTP endpoint, there are edge cases where we may want to use urn:ietf:params:ppm:error:unrecognizedMessage, but not know the task ID, for example, if a request body is too short to encode a task ID. With the present text, I think aggregators would have to respond with a non-PPM error type, or just a 400 Bad Request response with an empty body.

As "taskid" is the only extension member we define on our problem types, I think we should clarify the MUST requirement here, and only require it when the "type" is one of the error types in the PPM URN namespace.

For the edge cases where "taskid" is not known we could relax the MUST requirement, and only require a "taskid" member when the PPM Task ID is known. Alternately, we could suggest that the error in such error cases be of type about:blank (the default type, indicating no additional semantics beyond that of the HTTP status code) and with an appropriate status line, like "400 Bad Request".

cjpatton commented 2 years ago

As a consequence of the current text, PPM aggregators would always provide the same URI in the "instance" field. I would recommend that we remove the MUST requirement here.

+1, I think this should be MAY.

I think we should add a recommendation that, when one of the error types from Table 1 is used, the accompanying description from the table (or a localization thereof) should be provided in the "title" member. The "detail" member could then provide more particularized information, for example, a message mentioning the old and new configuration IDs involved in an urn:ietf:params:ppm:error:outdatedConfig error.

This sound fine.

As "taskid" is the only extension member we define on our problem types, I think we should clarify the MUST requirement here, and only require it when the "type" is one of the error types in the PPM URN namespace.

+1 to clarifying the edge case you point out, however I don't think this field should be required. MAY or SHOULD would be sufficient.