Support customized Cid generation #851

liwenjieQu · 2020-09-17T19:19:09Z

Currently Quinn generates cid using random number only. Some use case may require a more sophisticated cid structure. This change mainly add a ConnectionIdGenerator trait that allows to use any customized cid generator.

Ralith

Thanks for the PR! I agree that we should expose something like this, e.g. to support load balancers. Out of curiosity, are you doing something with Quinn that requires this in practice?

quinn-proto/src/config.rs

quinn-proto/src/shared.rs

quinn-proto/src/lib.rs

Ralith

Thanks for the updates! Made a deep pass; mostly cosmetic issues.

quinn-proto/src/config.rs

quinn-proto/src/endpoint.rs

quinn-proto/src/lib.rs

quinn-proto/src/shared.rs

quinn-proto/src/cid_generator.rs

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

quinn-proto/src/cid_generator.rs

quinn-proto/src/lib.rs

quinn-proto/src/cid_generator.rs

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Ralith

Thanks for your work!

liwenjieQu · 2020-09-28T17:33:20Z

Appreciate your time @Ralith !

liwenjieQu · 2020-09-28T18:37:19Z

Could you squash and merge these commits so I can push a new PR? Thanks

Ralith · 2020-09-28T18:43:14Z

I'd prefer to get a signoff from @djc first. You can make a new PR based on this branch, and when we merge this to master you should be able to rebase the new PR onto master without issue, and I'll happily review the new parts in the mean time. You could also squash this branch into a single commit yourself first for extra insurance against git getting tangled up.

…to implement CID generator

liwenjieQu · 2020-09-29T05:28:44Z

Just realize that I need to remove 'crate' from pub(crate) before new function for ConnectionId, and thus a third-party cid generator trait can implement generate_cid on its own

Ralith · 2020-09-29T05:44:49Z

Whoops, good catch.

liwenjieQu · 2020-09-29T05:55:30Z

The error from CI is mysterious. I am using a Mac and all tests pass on my side.

Ralith · 2020-09-29T05:59:50Z

There's a similar issue on #850; I'm guessing it's on github's end and we should ignore it.

liwenjieQu · 2020-09-29T21:21:57Z

Please not merge the PR now. I need to update the CID generator trait for proactive cid rotation liwenjieQu#1 where the lifetime/duration of a CID needs to be returned. I think it is better to declare in the very beginning to avoid breaking compatibility

…t breaks compatibility

liwenjieQu · 2020-09-29T22:43:31Z

Sorry for the inconvenience that the PR has to be reviewed again.

Ralith

Good catch. Minor API breakage isn't the end of the world at present, but might as well avoid it when we can.

quinn-proto/src/cid_generator.rs

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

djc · 2020-10-03T11:29:36Z

Sorry for the delayed review -- thanks for working on this, it looks pretty good!

infinity0 · 2021-03-04T14:51:03Z

quinn-proto/src/endpoint.rs

@@ -341,7 +345,7 @@ where
        if self.is_full() {
            return Err(ConnectError::TooManyConnections);
        }
-        let remote_id = ConnectionId::random(&mut self.rng, MAX_CID_SIZE);
+        let (remote_id, _) = RandomConnectionIdGenerator::new(MAX_CID_SIZE).generate_cid();


@liwenjieQu Is there any reason we hardcode the generator here, and don't use the local_cid_generator? The main difference I see between RandomConnectionIdGenerator vs local_cid_generator is the cid_len.

Here we pass remote_id to add_connection. However inside that function, we pass self._local_cid_generator to Connection::new which uses its length, instead of the length of remote_id (which was generated by RandomConnectionIdGenerator).

Should we change this to be self.local_cid_generator.generate_cid() for consistency.?

Note the name remote_id; we're generating a destination connection ID here, not a local one. The client uses a random destination connection ID for its initial flight of packets; see https://quicwg.org/base-drafts/draft-ietf-quic-transport.html#name-negotiating-connection-ids for details.

@Ralith Thanks for the explanation! That clears it up.

Support customized Cid generation

83b2ab7

Ralith reviewed Sep 17, 2020

View reviewed changes

quinn-proto/src/config.rs Outdated Show resolved Hide resolved

Matthias247 reviewed Sep 18, 2020

View reviewed changes

quinn-proto/src/config.rs Outdated Show resolved Hide resolved

quinn-proto/src/shared.rs Outdated Show resolved Hide resolved

quinn-proto/src/shared.rs Outdated Show resolved Hide resolved

Matthias247 reviewed Sep 18, 2020

View reviewed changes

quinn-proto/src/shared.rs Outdated Show resolved Hide resolved

Wenjie Li added 3 commits September 18, 2020 11:07

Add a setter for cid generator in EndpointConfig

7d755f8

Move cid generator to a module and export trait

0d07f96

Rework cid generator doc

88f45a3

liwenjieQu requested a review from Ralith September 18, 2020 19:06

Matthias247 reviewed Sep 18, 2020

View reviewed changes

quinn-proto/src/lib.rs Outdated Show resolved Hide resolved

Wenjie Li added 2 commits September 18, 2020 14:14

Minor formatting change

49b89e7

Resolve lint issue by using trait object

138ffbe

Ralith requested changes Sep 19, 2020

View reviewed changes

liwenjieQu and others added 4 commits September 21, 2020 09:18

Update quinn-proto/src/config.rs

5dd24ee

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Update quinn-proto/src/config.rs

3804958

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Update quinn-proto/src/endpoint.rs

a2329fb

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Minor api and doc change

d177497

liwenjieQu requested a review from Ralith September 21, 2020 18:56

Ralith requested changes Sep 22, 2020

View reviewed changes

liwenjieQu and others added 4 commits September 22, 2020 13:39

Update quinn-proto/src/cid_generator.rs

7988e7d

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Update quinn-proto/src/cid_generator.rs

e9ba9c5

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Update quinn-proto/src/cid_generator.rs

c2c8976

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Minor edits

6eb8444

liwenjieQu requested a review from Ralith September 22, 2020 22:07

Remove validate_cid API from cid generator

8b1978b

Ralith previously approved these changes Sep 28, 2020

View reviewed changes

liwenjieQu mentioned this pull request Sep 28, 2020

Proactive CID rotation liwenjieQu/quinn#1

Closed

remove pub(crate) from CID new function to allow a third-party crate …

394e995

…to implement CID generator

liwenjieQu dismissed Ralith’s stale review via 394e995 September 29, 2020 05:27

liwenjieQu requested a review from Ralith September 29, 2020 05:29

Add doc for cid new function

f5b2e77

Ralith previously approved these changes Sep 29, 2020

View reviewed changes

Add lifetime to CID to avoid future change on CID generator trait tha…

9947411

…t breaks compatibility

liwenjieQu dismissed Ralith’s stale review via 9947411 September 29, 2020 22:37

liwenjieQu requested a review from Ralith September 29, 2020 22:43

Ralith requested changes Sep 30, 2020

View reviewed changes

quinn-proto/src/cid_generator.rs Outdated Show resolved Hide resolved

Update quinn-proto/src/cid_generator.rs

c5fde8a

Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>

Ralith approved these changes Sep 30, 2020

View reviewed changes

djc merged commit 37a2fde into quinn-rs:master Oct 3, 2020

djc added a commit that referenced this pull request Oct 3, 2020

Style tweaks from #851

7073b6f

Ralith pushed a commit that referenced this pull request Oct 3, 2020

Style tweaks from #851

d79d548

liwenjieQu mentioned this pull request Oct 4, 2020

Proactive CID rotation #860

Merged

liwenjieQu mentioned this pull request Nov 24, 2020

Expose cid genearator customization API in endpoint builder #925

Merged

infinity0 reviewed Mar 4, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support customized Cid generation #851

Support customized Cid generation #851

liwenjieQu commented Sep 17, 2020

Ralith left a comment

Ralith left a comment

Ralith left a comment

liwenjieQu commented Sep 28, 2020

liwenjieQu commented Sep 28, 2020

Ralith commented Sep 28, 2020 •

edited

Loading

liwenjieQu commented Sep 29, 2020

Ralith commented Sep 29, 2020

liwenjieQu commented Sep 29, 2020

Ralith commented Sep 29, 2020

liwenjieQu commented Sep 29, 2020

liwenjieQu commented Sep 29, 2020

Ralith left a comment

djc commented Oct 3, 2020

infinity0 Mar 4, 2021

Ralith Mar 5, 2021

infinity0 Mar 5, 2021

Support customized Cid generation #851

Support customized Cid generation #851

Conversation

liwenjieQu commented Sep 17, 2020

Ralith left a comment

Choose a reason for hiding this comment

Ralith left a comment

Choose a reason for hiding this comment

Ralith left a comment

Choose a reason for hiding this comment

liwenjieQu commented Sep 28, 2020

liwenjieQu commented Sep 28, 2020

Ralith commented Sep 28, 2020 • edited Loading

liwenjieQu commented Sep 29, 2020

Ralith commented Sep 29, 2020

liwenjieQu commented Sep 29, 2020

Ralith commented Sep 29, 2020

liwenjieQu commented Sep 29, 2020

liwenjieQu commented Sep 29, 2020

Ralith left a comment

Choose a reason for hiding this comment

djc commented Oct 3, 2020

infinity0 Mar 4, 2021

Choose a reason for hiding this comment

Ralith Mar 5, 2021

Choose a reason for hiding this comment

infinity0 Mar 5, 2021

Choose a reason for hiding this comment

Ralith commented Sep 28, 2020 •

edited

Loading