Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use HTTP POST for report uploads #543

Merged
merged 1 commit into from Jan 25, 2024
Merged

Conversation

tgeoghegan
Copy link
Collaborator

@tgeoghegan tgeoghegan commented Jan 9, 2024

Per httpdir early review of DAP (1), it's inappropriate to use PUT for a resource that cannot later be GET. Change the HTTP method for report uploads to POST, and add a note making it clear that uploads are idempotent and safe to retry.

Previous description of this PR:
For that reason, and also consistency with the aggregation job and collection job request paths, hoist the report ID out of the upload request body and into the request path. We don't want to repeat the report ID twice in one request, so struct Report no longer includes a struct Metadata and instead just puts the time inline in Report. struct ReportMetadata is still used in InputShareAad and elsewhere in DAP, so its definition is not changed, just moved farther down the document, and we explain how to construct it from the report request path and body.

With this change, there is now a unique URI for each report uploaded to a DAP leader. However we do not add any requirement that an Aggregator support GET requests on that path, because not all DAP implementations will necessarily store enough per-report information to be able to respond to such a request with a complete struct Report. Implementations are free to do that, but can also respond with HTTP 204, 404, 410 or some other error if they wish. As these are well-established HTTP semantics, DAP doesn't need to spend any more ink on explaining them.

Copy link
Collaborator

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change is fine. However, as discussed off-PR (pointed out by @simon-friedberger, I think), switching from PUT to POST might be a smaller change. Are there downsides to using POST?

@tgeoghegan
Copy link
Collaborator Author

I lean toward using PUT and changing the request path because:

  1. It makes the report upload request path consistent with the aggregation job and collection job request paths, which include their respective unique job IDs.
  2. Using PUT elegantly makes it clear that this request is idempotent and safe to retry (of course if we go to a POST we can just explain in the DAP text that it's an idempotent POST).
  3. While this forces a wire-breaking change, I think we're going to do draft 10 of DAP anyway, and that will mean a wire-breaking change to update the HPKE domain separation strings. Making this change to request paths and message formats isn't zero risk, but I think it's a marginal lift for implementations if they're breaking wire compatibility anyway.

@wangshan
Copy link
Contributor

@tgeoghegan I read through the mailing list but don't get why this change is necessary? it seems specifying idempotent POST will do the same? Will we ever have a good reason to support GET a report?

struct {
ReportID report_id;
Time time;
} ReportMetadata;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does ReportMetadata still have meaning? would it be simpler if we simply put reportID and time in InputShareAad?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

struct ReportMetadata ends up being useful in the aggregation interaction (e.g. it goes into a ReportShare) so I think it's useful.

@tgeoghegan
Copy link
Collaborator Author

@tgeoghegan I read through the mailing list but don't get why this change is necessary? it seems specifying idempotent POST will do the same? Will we ever have a good reason to support GET a report?

I agree that this change is not necessary, in that the protocol will work fine if we do nothing, or if we switch to a POST request and otherwise change nothing. I laid out my arguments for this change above, and concede that my arguments are essentially esthetic.

FWIW I don't intend to move forward with this change until and unless we establish WG rough consensus for it. I'll be following up on the mailing list with some concise discussion points to move this and other topics forward.

@branlwyd
Copy link
Collaborator

For what it's worth, I think a POST is ultimately a smaller change that arrives at the desired behavior.

I don't see much use for a GET of uploaded reports, and implementations will plausibly delete report content as soon as possible to reduce storage costs (which would make GET not so useful).

@cjpatton
Copy link
Collaborator

@tgeoghegan please rebase when you have a moment

@tgeoghegan tgeoghegan changed the base branch from timg/error-consistency to main January 17, 2024 18:37
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objection to this change!

@tgeoghegan
Copy link
Collaborator Author

This change has been up for quite a while and it seems to me that the most popular option, by dint of its simplicity, is to change the request upload to using POST without changing the request path or the request body. Accordingly I'm repurposing this PR to make that change.

@tgeoghegan tgeoghegan changed the title Hoist report ID into upload request path Use HTTP POST for report uplodas Jan 24, 2024
@tgeoghegan tgeoghegan changed the title Use HTTP POST for report uplodas Use HTTP POST for report uploads Jan 24, 2024
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. I think this is slightly better because it's clearer that reports aren't necessarily GETable once they are PUT (or POSTed, now).

47 36 4c cf 1b c0 e3 af fc ca 68 73 c9 c3 81 f6 4a cd f9 02 06 62 f8 3f 46 c0 72
19 e7" and an aggregation job ID "95 ce da 51 e1 a9 75 23 68 b0 d9 61 f9 46 61
28" (32 and 16 byte octet strings, represented in hexadecimal), resource URI
`{helper}/tasks/{task-id}/aggregation_jobs/{report-id}` would be expanded into
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`{helper}/tasks/{task-id}/aggregation_jobs/{report-id}` would be expanded into
`{helper}/tasks/{task-id}/aggregation_jobs/{aggregation-job-id}` would be expanded into

(editorial nit)

Per httpdir early review of DAP ([1]), it's inappropriate to use PUT for
a resource that cannot later be GET. Change the HTTP method for report
uploads to POST, and add a note making it clear that uploads are
idempotent and safe to retry.

[1]: https://datatracker.ietf.org/doc/review-ietf-ppm-dap-09-httpdir-early-nottingham-2023-12-29/
@tgeoghegan tgeoghegan merged commit 8f1f29d into main Jan 25, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants