search

quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.54k stars 360 forks source link

proto: make PartialDecode API public #1865

Closed thynson closed 3 weeks ago

thynson commented 1 month ago

Context: #1860.

This PR did a bit more than just expose symbols to public, a trait ConnectionIdValidator was introduced, due to the fact that a QUIC Load balancer have to support different kind of Connection ID Schemes, per QUIC-LB-DRAFT Secion 4.1. Also some documents was added to suppress the clippy warning after those symbol being made public.

thynson commented 1 month ago

I'm not sure if ConnectionIdValidator should be move to quinn_proto::shared.

Ralith commented 1 month ago

What does this new trait add over the caller performing whatever validation they like themselves after decoding a header, as is already done with ConnectionIdGenerator::validate?

djc commented 1 month ago

What does this new trait add over the caller performing whatever validation they like themselves after decoding a header, as is already done with ConnectionIdGenerator::validate?

It looks like parsing the header needs the expected CID len as input.

thynson commented 1 month ago

Thanks for review. I'm going to polish it based on your comments

thynson commented 1 month ago

The idea of ConnectionIdValidator comes from s2n-quic, but they have Validator and Generator and Format: Validator + Generator(see here is what s2n-quic does).

While in quinn, ConnectionIdGenerator has validate for now, so the duplication of functionality seems unavoidable unless we change trait ConnectionIdGenerator to requires ConnectionIdParser. Shall we go in this way?

thynson commented 1 month ago

While in quinn, ConnectionIdGenerator has validate for now, so the duplication of functionality seems unavoidable unless we change trait ConnectionIdGenerator to requires ConnectionIdParser. Shall we go in this way?

After further digging, I believe ConnectionIdGenerator.parse and ConnectionIdValidator.validate provides different semantics. The former is used to decide whether a state-less reset should be sent, so rename ConnectionIdValidator to ConnectionIdParser seems right to me.

djc commented 3 weeks ago

I ended up squashing and reordering these changes. I changed the commits to (a) first make the APIs public and then (b) add the ConnectionIdParser trait and implementation. The previous approach had some intermediate states that did not make sense. @thynson can you check that this still satisfies your use case (there should be no changes in terms of the final public API, but still good to check)?

thynson commented 3 weeks ago

It's totally fine to me .