Migrate to Axum [part 2]: Migrate hpke_config to Axum by jcjones · Pull Request #4427 · divviup/janus

jcjones · 2026-03-05T17:37:22Z

This adds an axum_hpke_config handler function, wiring it into Axum.
It leaves behind the existing hpke_config and hpke_config_cors_preflight methods to support other unit tests that want them for now, but we'll clean that up in part 6.
Strips out part 1's demo test, and instead routes "/hpke_config" via axum::routing::get(axum_hpke_config::<C>).layer(hpke_cors), roughly like I do in Relay.
Pulled impl Error and into_response_with_retry_after out of Migrate to Axum #4409.

Partly authored by Claude

aggregator/src/aggregator/http_handlers.rs

tgeoghegan · 2026-03-05T22:44:26Z

aggregator/src/aggregator/http_handlers.rs

+    Ok((StatusCode::OK, response_headers, encoded_hpke_config_list).into_response())
+}
+
+// TODO(#4283): Remove in Part 6 when Trillium is fully removed.


Why not delete it (and hpke_config_cors_preflight) now?

tgeoghegan · 2026-03-05T22:45:51Z

aggregator/src/aggregator/http_handlers.rs

+
+    if let Some(signature) = signature {
+        response_headers.insert(
+            http::header::HeaderName::from_static(HPKE_CONFIG_SIGNATURE_HEADER),


Odd that we use http::header::HeaderValue but fully quality http::header::HeaderName here

This is a Claudeism and it snuck through me, sorry about that. Haste makes LLM slop.

aggregator/src/aggregator/http_handlers.rs

tgeoghegan · 2026-03-05T22:51:39Z

aggregator/src/aggregator/problem_details.rs


    #[tokio::test]
    async fn test_retry_after_header() {
+        use axum::response::IntoResponse;


Put this use at the top of the module.

tgeoghegan · 2026-03-05T23:00:17Z

aggregator/src/aggregator/problem_details.rs

+    }
+}
+
+impl IntoResponse for &ProblemDocument<'_> {


If you implement this for ProblemDocument then you won't have to awkwardly do (&doc).into_response() all over Error::to_response

🤦🏼 yeah!

tgeoghegan · 2026-03-05T23:03:15Z

aggregator/src/aggregator/problem_details.rs

+    }
+
+    /// Converts this problem document into an axum [`Response`].
+    pub fn into_response_with_retry_after(&self, retry_after: Option<Duration>) -> Response {


Generally, naming something into implies that you move the value in ("A value-to-value conversion that consumes the input value."). It doesn't look like this moves any part of self into the returned value.

:( sorry. It's now to_response_with_retry_after

aggregator/src/aggregator/problem_details.rs

tgeoghegan · 2026-03-05T23:07:44Z

aggregator/src/aggregator/http_handlers.rs

+                .with_task_id(&inner.task_id)
+                .with_detail(&inner.to_string()))
+                .into_response(),
+            Error::Datastore(error @ datastoreError::TimeUnaligned { task_id, .. }) => {


Hmm, it's orthogonal to this PR, but is this error possible anymore? #4430

aggregator/src/metrics/tests/prometheus.rs

- This adds an `axum_hpke_config` handler function, wiring it into Axum. - It leaves behind the existing `hpke_config` and `hpke_config_cors_preflight` methods to support other unit tests that want them for now, but we'll clean that up in part 6. - Strips out part 1's demo test, and instead routes `"/hpke_config"` via `axum::routing::get(axum_hpke_config::<C>).layer(hpke_cors)`, roughly like I do in Relay. - Pulled `impl Error` and `into_response_with_retry_after` out of #4409. Partly authored by Claude

tgeoghegan

It's nice that this doesn't penetrate deeper into the call stack below aggregator.handle_hpke_config(). 🤞🏻 that the rest of our endpoints are similarly well-layered.

tgeoghegan · 2026-03-10T23:04:38Z

aggregator/src/aggregator/problem_details.rs

+
+    /// Converts this problem document into an axum [`Response`].
+    pub fn to_response_with_retry_after(&self, retry_after: Option<Duration>) -> Response {
+        let body = serde_json::to_vec(self).unwrap_or_default();


Maybe a nit: should we be suppressing failures to encode problem documents as JSON? Default here will be an empty Vec and then we'll send out a malformed response that has Content-Type: application/json+problem_document (or whatever it exactly is) and no body, right?

OK, this calls for warn!. Let's try:

let body = match serde_json::to_vec(self) { Ok(body) => body, Err(e) => { warn!(?e, ?self, "Failed to serialize problem document"); "{}".as_bytes().to_vec() } };

tgeoghegan · 2026-03-10T23:06:38Z

aggregator/Cargo.toml


 [dev-dependencies]
 janus_aggregator = { workspace = true, features = ["fpvec_bounded_l2", "test-util"] }
+tower.workspace = true


We already have tower = { workspace = true } above in regular dependencies and this doesn't add any features. I think we can delete it?

Rebase fail, thanks!

tgeoghegan · 2026-03-10T23:13:50Z

aggregator/src/aggregator/http_handlers.rs

 /// are tightly scoped to relevant endpoints, and our CORS settings are unlikely to change.
 /// See: <https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Max-Age>.
-const CORS_PREFLIGHT_CACHE_AGE: u32 = 24 * 60 * 60;
+const CORS_PREFLIGHT_CACHE_AGE: StdDuration = StdDuration::from_secs(24 * 60 * 60);


I'm 99.9% this is good and right (it's definitely better than a u32) but just so we thought about it: in other places like the datastore, we've moved toward using chrono types like TimeDelta. If we did that here, would that just cause friction when we try to stick this value into HTTP headers or something-something-CORS?

I used StdDuration because it's already used all over this file, but hmm!

No, ugh:

the trait `From<chrono::TimeDelta>` is not implemented for `tower_http::cors::MaxAge`

No feature for chrono types either: https://docs.rs/crate/tower-http/latest/features

jcjones added this to the 2026Q1 Janus Release milestone Mar 5, 2026

jcjones marked this pull request as ready for review March 5, 2026 17:39

jcjones requested a review from a team as a code owner March 5, 2026 17:39

jcjones force-pushed the 4283-migrate-to-axum-part-2 branch from 1064aed to 81087b8 Compare March 5, 2026 21:40

tgeoghegan mentioned this pull request Mar 5, 2026

Migrate to Axum [part 1]: Add axum scaffold with dual-listener proxy bridge #4426

Merged

tgeoghegan requested changes Mar 5, 2026

View reviewed changes

jcjones force-pushed the 4283-migrate-to-axum-part-2 branch from 81087b8 to 4ad0c58 Compare March 9, 2026 19:15

jcjones requested a review from tgeoghegan March 9, 2026 20:55

Base automatically changed from 4283-migrate-to-axum-part-1 to main March 10, 2026 22:34

jcjones added 3 commits March 10, 2026 15:43

Review updates

bd4b2a5

Use tower::ServiceExt::oneshot in test_retry_after_header

32c0934

jcjones force-pushed the 4283-migrate-to-axum-part-2 branch from 4ad0c58 to 32c0934 Compare March 10, 2026 22:52

tgeoghegan approved these changes Mar 10, 2026

View reviewed changes

tgeoghegan reviewed Mar 10, 2026

View reviewed changes

review updates!11oneone

f73d231

divergentdave approved these changes Mar 13, 2026

View reviewed changes

jcjones merged commit d5c7df8 into main Mar 13, 2026
8 checks passed

jcjones deleted the 4283-migrate-to-axum-part-2 branch March 13, 2026 18:23

Conversation

jcjones commented Mar 5, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tgeoghegan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants