Re: Httpdir early review of draft-ietf-ppm-dap-09

[Tim Geoghegan <[e-mail]>]

Thank you for the thoughtful review, Mark, and especially for getting us
early feedback so quickly. It's also gratifying to know we're on the right
track! I have posted a pull request (
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540) to the draft to
address the items that I think are straightforward and non-controversial
(though naturally review of those changes and further discussion is
welcome). Beyond that, I have some comments inline:

On Fri, Dec 29, 2023 at 12:57 AM Mark Nottingham via Datatracker <
[e-mail]> wrote:

> - 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).
>

Addressed in commit 5dba50dd0455d3668c8e469f5fc2072db868673c in
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540

- 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').
>

Addressed in commit 83dc24deb09ae2e810c1f8dac4cea96e907da7c1 in
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540

- 'carried over HTTPS' isn't correct; should be 'carried over HTTP'. You
> later
> specify use of HTTPS as a requirement.
>

Addressed in commit 5c09e813c66f38081b530acd566afcc9d469e0b4 in
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540


> - throughout, you use 'sub-protocol'. This is likely to confuse; suggest
> 'interaction' or similar (which is already in use, e.g., in section 4).
>

Addressed in commit 4bcd0a79ccaee41808dbf9e4404355c4d385171a in
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540


> - '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..
>

Good point. The other problem is that I don't think the reference to a
"challenge object" makes any sense. What we mean is that a message in the
response body might contain error information. So we need to fix that text.
That aside, would it suffice to amend this section to say that if the
object in the response body indicates an error, then the HTTP status MUST
indicate either a client or server error?


> - In section 4.3, it might be good to reference RFC 6570.
>

Speaking as the individual who wrote the current text: I deliberately
avoided using RFC 6570 here because its feature set is much, much richer
than what DAP needs, and so I opted for the weaker, simpler templates that
we have now.

Speaking as a document editor/WG member: I have no doubt that the templates
and expansions DAP uses can be expressed in RFC 6570 syntax, and will
switch over to it if the WG consensus is to do so, but personally I don't
see a compelling reason to go that way (I'm open to hearing arguments about
this of course).


> - 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.
>

We tried to write the text to allow serving of the API relative to any
prefix. So in a template like "{aggregator}/hpke_config" (4.4.1),
"{aggregator}" can have arbitrary path components in it relative to the
hostname. Did we express this incorrectly?

Past that, are you objecting to DAP spelling out templates like
"{aggregator}/hpke_config" or
"{helper}/tasks/{task-id}/aggregation_jobs/{aggregation-job-id}"? Are you
suggesting that a deployment should be able to use a template like
"{helper}/foo/{task-id}/bar/{aggregation-job-id}" if it so wishes?


> - 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.
>

Addressed in commit 52cc0134182c1542815d2262fe0bef07a2a18f0f in
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540


> - 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.
>

Addressed in commit 7aa8141f54ad624a9036a7abcfbe4f328db10bcf in
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/540


> - 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.
>

First, there is a unique identifier for a report that does get chosen by
the client, but it only appears inside the request body (this is
`Report.report_metadata.report_id`). We could hoist it up to the request
path, yielding a template `{leader}/tasks/{task-id}/reports/{report-id}`,
and then you'd have a unique URI for each report. I'm inclined to do this
much no matter what, for consistency with the aggregation job and
collection job resource paths.

That said, I'm not sure if it makes sense for that to be a GETtable
resource. DAP doesn't need GET on reports to function and more importantly,
I worry about the implementation burden for servers. In many cases, the
initial uploaded form of a report isn't useful after the first round of
preparation. Servers may wish to discard per-report information
aggressively to limit the growth of their databases, which makes it
difficult to serve up anything meaningful in response to a GET request. So
I'm reluctant to add a requirement or a MUST about GET on this resource.

I really like having this be a PUT request because of the implied
idempotency semantics. What if we leave it as a PUT and change the path
template to incorporate the unique report ID, but then don't say anything
about GET requests (or at most put in a MAY)? I think then implementations
would be free to either respond to GET requests with HTTP 200 and a `struct
Report`, or with HTTP 204 or 410 if they happen to have chucked out the
relevant database row.

If that's inappropriate, then I think we can switch to a POST as you
recommend, but what about idempotency? Would we just have to state in DAP
that this particular POST is safe to retry?


> - 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).
>

Besides the fact that aggregation job paths already incorporate the unique
aggregation job ID, I have the same thoughts here as I did for the upload
endpoints:

- the protocol does not require a GETtable aggregation job resource to
function
- implementations are free to wire that up if they want to
- I find it useful to be able to use the HTTP method to distinguish between
"idempotently create this aggregation job" (PUT) and "mutate the existing
aggregation job by advancing its state" (POST)

However we could make this all POST, all the time, and have aggregators key
off of the media type to decide which handler to invoke.


> - Use of 201 in 4.6.1 seems incorrect if I understand correctly.
>

The request in question is PUT
{leader}/tasks/{task-id}/collection_jobs/{collection-job-id}, which is
intended to create a collection job resource with the specified ID, in the
specified task. RFC 9110 9.3.4 (
https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.4-2) states:

If the target resource does not have a current representation and the PUT
successfully creates one, then the origin server MUST inform the user agent
by sending a 201 (Created) response.


So 201 seems correct: on success, the collection job gets created. The same
paragraph in 9110 discusses using 200 or 204 when successfully mutating an
existing resource, but DAP explicitly forbids that in this request.

Thanks again,
Tim