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

upload: Mention that expired tasks might be treated as unrecognized #641

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

cjpatton
Copy link
Collaborator

@cjpatton cjpatton commented Jan 14, 2025

Stacked on #639.

Clients are free to pick any timestamp they wish, even one in the
validity range of a task that has expired. The Aggregators need to
prevent themselves from aggregating such reports indefinitely.

Add some text to the upload flow indicating that indicating an
unrecognized task may suggest the client is either misconfigured or
misbehaving.

@cjpatton cjpatton marked this pull request as ready for review January 14, 2025 18:19
@cjpatton
Copy link
Collaborator Author

Question for reviewers: if we take this change for upload and aggregation, should we also do the same for collection?

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.

The Aggregators need to prevent themselves from aggregating such reports indefinitely.

I think the Aggregators will generally evict "old" state (for some implementation-defined definition of "old"), which will effectively avoid the risk of needing to receive such reports indefinitely since reports touching on evicted state MUST be dropped with report_dropped. However, evicting old state is not required by DAP, so this change SGTM.

Question for reviewers: if we take this change for upload and aggregation, should we also do the same for collection?

I don't see why not. We might want to add an additional note reminding implementors that the leeway is even more important in this case, since collection typically must occur after the batch interval, so without appropriate leeway the final batch might be uncollectable.

@tgeoghegan
Copy link
Collaborator

I'm fine with this in the upload interaction, but not in the aggregation or collection interactions. Task expiry is about limiting which client reports get aggregated into a task, but it seems reasonable to allow aggregation and collection to occur after aggregators stop accepting reports. If we forbid aggregation or collection as aggressively as this change suggests, then we run the risk of orphaning valid, aggregatable reports.

I would think that garbage collection accounts for this on its own: an aggregator will throw away reports if the task they belong to is expired for too long, at which point the aggregation or collection fails as described in {#input-share-validation}. I'd support adding an implementor's note discussing this, or something in operational considerations.

draft-ietf-ppm-dap.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

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

GitHub requires that I put something here if I choose "Request changes" but see my comments above

@cjpatton cjpatton force-pushed the cjpatton/time-3 branch 2 times, most recently from 0e9b10f to f7cfe33 Compare January 15, 2025 15:47
@cjpatton
Copy link
Collaborator Author

2025/1/15 call: don't abort during aggregation or collection: (1) in practice there is backlog to process for expired tasks; (2) we already allow dropping reports when the releavant replay state is evicted.

Let's keep this for upload, so the Leader has a tool to deal with malicious Clients, but disallow for aggreagtion/collection.

draft-ietf-ppm-dap.md Outdated Show resolved Hide resolved
@cjpatton cjpatton marked this pull request as draft January 15, 2025 16:47
@cjpatton cjpatton changed the base branch from cjpatton/time-3 to cjpatton/time-2 January 15, 2025 22:18
@cjpatton cjpatton added editorial The issue raised is purely editorial (i.e., doesn't impact implementations). and removed feature labels Jan 15, 2025
@cjpatton cjpatton changed the title Allow abort for requests for expired tasks upload: Mention that expired tasks might be treated as unrecognized Jan 15, 2025
Clients are free to pick any timestamp they wish, even one in the
validity range of a task that has expired. The Aggregators need to
prevent themselves from aggregating such reports indefinitely.

Add some text to the upload flow indicating that indicating an
unrecognized task may suggest the client is either misconfigured or
misbehaving.
@cjpatton cjpatton marked this pull request as ready for review January 15, 2025 22:27
@cjpatton cjpatton requested a review from tgeoghegan January 15, 2025 22:27
@cjpatton cjpatton merged commit 647745d into cjpatton/time-2 Jan 23, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
draft-14 editorial The issue raised is purely editorial (i.e., doesn't impact implementations).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants