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

[Tim Geoghegan <[e-mail]>]

As promised/threatened, some additional PRs that came out of this
discussion:

require error responses to be consistent -
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/542
Hoist report ID into upload request path -
https://github.com/ietf-wg-ppm/draft-ietf-ppm-dap/pull/543

Tim

On Tue, Jan 9, 2024 at 8:40 AM Tim Geoghegan <[e-mail]> wrote:

> Thanks again for sharing your thoughts. I am working on some further DAP
> changes to implement more of your recommendations and will follow up here
> with pull requests links as I post them. Just a few more comments inline,
> below.
>
> Tim
>
> On Sun, Jan 7, 2024 at 4:59 PM Mark Nottingham <[e-mail]> wrote:
>
>> On 3 Jan 2024, at 8:06 am, Tim Geoghegan <[e-mail]> wrote:
>> >
>> > - '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?
>>
>> That's one way to do it, yes -- although that might be too constraining
>> (e.g. when it's an error that doesn't have impact on / significance to HTTP
>> components).
>>
>
> I understand the distinction you're making between the HTTP/transport
> layer and the DAP/application layer, but personally I dislike APIs that
> indicate success at the transport layer but then an error in the message,
> because inevitably clients only look at the HTTP status. For instance the
> infamous AWS S3 CompleteMultipartUpload, which will give you a 200 OK but
> then only in the response body will tell you that your object was not
> actually committed to storage (
> https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html).
> This has burnt pretty much everyone who uses the S3 HTTP API. The official
> AWS SDKs do the right thing for you, but if you're using those, I'd argue
> you're not really interacting with the HTTP API but rather a
> Python/Java/Go/whatever abstraction over it, with its own error
> representation and handling concepts. But I digress...
>
>
>> > - 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).
>>
>> AIUI these are just an editorial convention / explanatory device, so I
>> don't feel strongly about this, but 6570 does designate "level 1" for such
>> simple cases.
>>
>
> Oh, that's neat, I hadn't realized it offered different feature sets. I'll
> give RFC 6570 another read and see what adopting it for our URL templates
> would look like.
>
> > - 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?
>>
>> Using prefixes is better than static URLs. Ideally, the client would
>> fetch something that contains the links (or templates for them) to allow
>> complete flexibility, much as HTML does for the Web. Have you considered
>> that approach?
>>
>
> I assume you're talking about something like the ACME directory (
> https://datatracker.ietf.org/doc/html/rfc8555#section-7.1.1)? Yes, the
> DAP authors are aware of that design. Personally I don't think it's right
> for DAP. I may be revealing my inexperience as an API designer and server
> operator but I don't see how the flexibility in request path layout is
> worth the additional complexity of a directory endpoint. Additionally, in a
> protocol like DAP, which will see far, far greater volume of traffic than
> ACME, forcing an extra HTTP round trip for some report uploads -- to check
> where HPKE configs are and where uploads should go -- seems wasteful.
>
> But of course that's my opinion as an individual WG member. If someone in
> the WG wants to champion the idea of a directory and builds rough consensus
> around the idea, then I'd support it.
>
>
>> > - 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.
>>
>> If you do that, PUT makes sense.
>>
>> > 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.
>>
>> Understood, but I wonder how much of a burden it really is for a server
>> to return some state that it has sitting there already.
>>
>> > 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.
>>
>> Yes, if the resource isn't available any more, 404 or 410 is entirely
>> appropriate. PUT isn't a guarantee that the same bytes will always be
>> returned -- or even returned on the next request. However, it should make
>> sense, from a systemic view.
>>
>> > 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?
>>
>> Well, this is under development and should ship soon: <
>> https://datatracker.ietf.org/doc/draft-ietf-httpapi-idempotency-key-header/
>> >
>>
>
> In our case, we don't need a synthetic idempotency key because we already
> have a natural one: the unique report ID. I meant that by switching to
> POST, we'd lose the inherent implication that the request is safe to retry,
> and I was asking how protocols should make it clear that a POST request is
> idempotent. In any case I think this is moot, because we'll stick with PUT
> for this request and move the unique identifier into the request path.
>
>
>> > - 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.
>>
>> Ah, sorry, I misread that.
>>
>> Cheers,
>>
>> --
>> Mark Nottingham   https://www.mnot.net/
>>
>>