Task rewrite: introduce new task types #2016
Conversation
We want to enable aggregators to only store authentication token hashes
if they don't need the actual token (e.g., a helper only receives
aggregation sub-protocol messages). With the existing
`janus_aggregator_core::task::Task` structure, this is awkward, because
that struct was used both as a view into all of a task's parameters and
secrets, useful for test code that orchestrates the provisioning and
execution of a task, as well as a specific DAP participant's view of the
task, which is useful in Janus itself.
To resolve that tension, this commit introduces distinct structures for
those distinct views. `janus_aggregator_core::task::test_util::Task`
contains all of a task's parameters and secrets, regardless of any
protocol role. `janus_aggregatore_core::task::test_util::NewTaskBuilder`
is used in test contexts to build `test_util::Task` values, and then
`test_util::Task::{leader_view, helper_view}` then allow obtaining the
leader or helper specific view of the task as a
`janus_aggregator_core::task::AggregatorTask`. This holistic view of a
task is only used in test code, and thus we confine it to the
`test_util` module.
`AggregatorTask` is an aggregator's view of the task, and uses an enum
to represent the leader, helper or taskprov-provisioned helper specific
parameters. This task representation is what gets used by non-test
configurations of Janus. We also add `SerializediAggregatorTask`, which
much like `SerializedTask` is an adapter so that `AggregatorTask` values
can be serialized to and from YAML, for use in `janus_cli`.
The goal is to migrate all of Janus from the existing
`janus_aggregator_core::task::Task` and
`janus_aggregator_core::task::test_util::TaskBuilder` onto
`task::test_util::Task`, `task::test_util::NewTaskBuilder` and
`task::AggregatorTask`. Doing this all in one go would yield a huge diff
that is difficult to review, so in order to enable incremental adoption
of the new structures throughout Janus, we have them co-exist for now.
Once all usages of `task::Task` and `task::test_util::TaskBuilder` have
been removed, those types can be deleted and `NewTaskBuilder` can be
renamed to just `TaskBuilder`.
To see where all this is going, check out #2014.
Part of #1524
aggregator_core/src/task.rs
Outdated
| .map(|keypair| (*keypair.config().id(), keypair)) | ||
| .collect(); | ||
|
|
||
| common_parameters.validate()?; |
There was a problem hiding this comment.
It seems out of place that this is in a constructor instead of AggregatorTask::validate(), or instead of having the constructor check all validation constraints.
It looks like our old Task struct didn't validate on construction largely because it was internally used as storage inside TaskBuilder, so we intentionally initialized it with inconsistent values, wrote to fields without re-checking invariants, etc., and then only validated parameters on conversion from a TaskBuilder into the inner Task. Since AggregatorTask doesn't get used for that purpose anymore, we could make the validate() methods private, make both constructors fallible, and always validate before returning a CommonTaskParameters or AggregatorTask.
There was a problem hiding this comment.
I folded the validate methods into the constructors, since validation never needs to be done independently of instantiation of a CommonTaskParameters or AggregatorTask value.
aggregator_core/src/task.rs
Outdated
| /// Return the HPKE keypairs used by this aggregator to decrypt client reports, or `None` for | ||
| /// taskprov tasks. |
There was a problem hiding this comment.
This should instead say that the HashMap will be empty for taskprov tasks.
- fold validate methods into constructors - fix comment
tgeoghegan
added
the
allow-changed-migrations
We want to enable aggregators to only store authentication token hashes if they don't need the actual token (e.g., a helper only receives aggregation sub-protocol messages). With the existing
janus_aggregator_core::task::Taskstructure, this is awkward, because that struct was used both as a view into all of a task's parameters and secrets, useful for test code that orchestrates the provisioning and execution of a task, as well as a specific DAP participant's view of the task, which is useful in Janus itself.To resolve that tension, this commit introduces distinct structures for those distinct views.
janus_aggregator_core::task::test_util::Taskcontains all of a task's parameters and secrets, regardless of any protocol role.janus_aggregatore_core::task::test_util::NewTaskBuilderis used in test contexts to buildtest_util::Taskvalues, and thentest_util::Task::{leader_view, helper_view}then allow obtaining the leader or helper specific view of the task as ajanus_aggregator_core::task::AggregatorTask. This holistic view of a task is only used in test code, and thus we confine it to thetest_utilmodule.AggregatorTaskis an aggregator's view of the task, and uses an enum to represent the leader, helper or taskprov-provisioned helper specific parameters. This task representation is what gets used by non-test configurations of Janus. We also addSerializediAggregatorTask, which much likeSerializedTaskis an adapter so thatAggregatorTaskvalues can be serialized to and from YAML, for use injanus_cli.The goal is to migrate all of Janus from the existing
janus_aggregator_core::task::Taskandjanus_aggregator_core::task::test_util::TaskBuilderontotask::test_util::Task,task::test_util::NewTaskBuilderandtask::AggregatorTask. Doing this all in one go would yield a huge diff that is difficult to review, so in order to enable incremental adoption of the new structures throughout Janus, we have them co-exist for now. Once all usages oftask::Taskandtask::test_util::TaskBuilderhave been removed, those types can be deleted andNewTaskBuildercan be renamed to justTaskBuilder.To see where all this is going, check out #2014.
Part of #1524