Early Review of draft-ietf-ppm-dap-09
review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29-00
Request | Review of | draft-ietf-ppm-dap |
---|---|---|
Requested revision | No specific revision (document currently at 13) | |
Type | Early Review | |
Team | HTTP Directorate (httpdir) | |
Deadline | 2024-01-31 | |
Requested | 2023-12-11 | |
Requested by | Benjamin M. Schwartz | |
Authors | Tim Geoghegan , Christopher Patton , Brandon Pitman , Eric Rescorla , Christopher A. Wood | |
I-D last updated | 2023-12-29 | |
Completed reviews |
Httpdir Early review of -09
by Mark Nottingham
(diff)
|
|
Comments |
An HTTPDIR review was specifically requested in relation to this question: https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/issues/450. Comments on other aspects of the document are also welcome. |
|
Assignment | Reviewer | Mark Nottingham |
State | Completed | |
Request | Early review on draft-ietf-ppm-dap by HTTP Directorate Assigned | |
Reviewed revision | 09 (document currently at 13) | |
Result | On the right track | |
Completed | 2023-12-29 |
review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29-00
I've had a fairly quick look through and copied my notes below. Happy to explain / expand upon / explore any or all as necessary. - Throughout, 'endpoint' is used to identify nodes. In most cases, this should be 'resource' in a HTTP protocol (although it might be 'server' in some of these cases). - the Overview uses a DAP-specific definition of 'client', but a HTTP definition of 'server'. These should be aligned. I'd suggest distinguishing where necessary with 'HTTP client', 'HTTP server', etc. (but see point above about 'resource'). - 'carried over HTTPS' isn't correct; should be 'carried over HTTP'. You later specify use of HTTPS as a requirement. - throughout, you use 'sub-protocol'. This is likely to confuse; suggest 'interaction' or similar (which is already in use, e.g., in section 4). - 'Errors can be reported in DAP both at the HTTP layer and within challenge objects as defined in Section 8.' It should be noted that for HTTP software to work correctly, the appropriate status code must be used; indicating an error in the object without doing so may lead to unintended retries/caching/etc. - In section 4.3, it might be good to reference RFC 6570. - Section 4.4.1 and afterwards use static paths for subresources, hindering flexibility and violating at least the spirit of BCP 190. If the configuration were to allow specification of URI templates, it would be much more flexible. - 4.4.1 also requires the client to abort if a GET request fails. In practice, GETs can and will be retried, sometimes automatically (by clients and/or infrastructure), so this will be impractical to enforce. - Also in 4.4.1 the cache-control example looks like it specifies a particular freshness lifetime; it would be good to explicitly say that is only an example. - 4.4.2 uses PUT to create a report, but doesn't seem to make the uploaded report available for later GETs at the same URL. If this stays the same (i.e., if the server is going to manage the URL of the report), it should be POST. It should only be PUT if the client is effectively deciding what the URL should be (by PUTting to a URL it can later GET). Also, if it can't be GET later, the response status code should be 200, not 201. If it can and POST is used, it should be 201 + a Location header pointing to where it was created. - Similar considerations for 4.5.1.1 and 4.5.1.2. In an nutshell, use PUT when you can expect to GET the same thing from the same URI later (delta whatever the server does to it in the meantime). Use POST when you want "process this", "this" being identified by the media type + URL, and it optionally has the side effect of creating a new resource at a URL that the server controls (indicated by 201 + Location). - Use of 201 in 4.6.1 seems incorrect if I understand correctly.