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

Handle collect requests properly #45

Closed
cjpatton opened this issue Jul 13, 2022 · 5 comments
Closed

Handle collect requests properly #45

cjpatton opened this issue Jul 13, 2022 · 5 comments
Assignees

Comments

@cjpatton
Copy link
Contributor

Daphne is currently missing ietf-wg-ppm/draft-ietf-ppm-dap#277, which is needed for privacy. However the exact requirements for collect requests (and constraints on them) are a bit of a moving target. (See ietf-wg-ppm/draft-ietf-ppm-dap#195.)

@cjpatton cjpatton added bug Something isn't working and removed bug Something isn't working labels Jul 21, 2022
@cjpatton
Copy link
Contributor Author

(This isn't really a bug per se, it's more of a feature parity issue.)

@cjpatton
Copy link
Contributor Author

One thought I had here while reviewing #63: The Leader should reject collect requests for which:

  • The batch interval is huge, in the sense that it spans more than, say, 100 time windows. Otherwise updating ReportStore buckets after a batch is aggregated will be expensive.
  • The start of the batch interval is too far in the past, say more than a few days. Otherwise we might end up holding onto a request for which we're not likely to get any data.
  • The end of the batch interval is too far in the future, say more than a few hours. Similar reasoning as above.

@cjpatton
Copy link
Contributor Author

Suggestion for how to proceed:

  • Mark each AggreagteStore DO instance as collected or not collected.
  • When a collect request comes in, for each batch window, check the AggreagteStore to see if it has been collected. If so, reject. Otherwise, if none have been collected, accept.

@nakatsuka-y
Copy link
Contributor

Closing as #63 has been merged to main branch.

@cjpatton cjpatton reopened this Aug 6, 2022
@cjpatton
Copy link
Contributor Author

cjpatton commented Aug 6, 2022

Ropened due to bug spotted after merging the main PR. This condition is wrong:
https://github.com/cloudflare/daphne/blob/main/daphne_worker/src/dap.rs#L358-L359

There is a similar issue on the Helper side.

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

No branches or pull requests

2 participants