Fix panic when trying to get HPKE configs on a task that doesn't have any configured #1738
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
inahga
commented
Aug 16, 2023
| // Assuming something hasn't gone horribly wrong with the database, this | ||
| // should only happen in the case where the system has been moved from taskprov | ||
| // mode to non-taskprov mode. Thus there's still taskprov tasks in the database. | ||
| // This isn't a supported use case, so the operator needs to delete these tasks |
There was a problem hiding this comment.
I've updated #1736 to back up this assertion.
tgeoghegan
approved these changes
Aug 16, 2023
inahga
added a commit
that referenced
this pull request
Aug 18, 2023
…er-01 (#1748) * Split aggregator_api/src/lib.rs (#1683) * Minor aggregator API cleanup (#1694) * Deletion should return 204 on not found, not 404. It's not really an error if we try to delete something and it's already been deleted. This also makes the deletion request idempotent. * Provide explicit return type in routes * Remove needless Halt I'm pretty sure this Halt isn't required and is just noisy. Other routes don't use it. * Aggregator API error handling cleanup (#1698) * Aggregator API error handling cleanup As this crate grows, having to map each error is becoming burdensome. Mainly turns `Error` into a enum from a struct. We can map errors into variants that handle HTTP status codes for us, and use `#[from]` to let us use `?` on datastore errors. We do lose a little bit of tracing in this PR. Datastore errors are unaffected, because datastore methods are already traced. But handler-local internal error paths aren't well traced, some context might be missing since we'll only see the final result. There's ways around this, but the internal error cases look to be _very_ unlikely to happen (and likely should be unwraps anyways), so I don't think it's worth spending much time on. * PR feedback * Fix flaky test that occasionally collides on random() (#1729) * Add peer aggregator endpoints to aggregator API (#1689) * Aggregator API endpoints for configuring taskprov peer aggregator * Remove querystring as a dependency * Align custom serde with other implementations * Clippy * Clippy lints that don't show up locally 🤷 * Provide role and endpoint in request body * Revert "Remove querystring as a dependency" This reverts commit 74de8a7. * Remove serde_urlencoded Cargo wants to remove random things 🤷 * Remove unneeded error * Fix panic when trying to get HPKE configs on a task that doesn't have any configured (#1738) * Document taskprov extension and global HPKE keys (#1736) * Document taskprov extension and global HPKE keys * Typo * Once you go taskprov, you never go back * Spacing Co-authored-by: Tim Geoghegan <[email protected]> * Typo Co-authored-by: Tim Geoghegan <[email protected]> * Wording Co-authored-by: Tim Geoghegan <[email protected]> * We're doing taskprov-04 for dap-02 --------- Co-authored-by: Tim Geoghegan <[email protected]> --------- Co-authored-by: Tim Geoghegan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supports #1568.
If a taskprov task is present, the end-user can still query
/hpke_configs?task_id=foo. If they do this, the handler will panic because we have anunwrapin the task-specifichandle_hpke_config()function. Taskprov tasks don't have any HPKE keys directly associated with them.Partially fix this by unconditionally returning the global keys when in taskprov mode, so any client
task_idparameters are ignored.This doesn't handle the case where a taskprov task is in the database, the system is out of taskprov mode, and a client queries
/hpke_configs?task_id=foo. I figure this is a niche enough case that we can ignore it, however I wrapped it in a comment and nicer error handling logic.This awkward logic goes away if we adopt #1641. I'm also open to cleaner ways of handling this case.