Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose function to convert byte slice to a connection ID #3614

Merged

Conversation

hf
Copy link
Contributor

@hf hf commented Nov 6, 2022

Although quic.Config exposes a ConnectionIDGenerator option, it's not usable outside of the quic-go implementation because type ConnectionID = protocol.ConnectionID and protocol is an internal package not accessible from the outside.

To fix this, the ReadConnectionID and ParseConnectionID methods from protocol are re-exported in the toplevel package.

@marten-seemann
Copy link
Member

What is ReadConnectionID needed for?

ParseConnectionID should probably also have better name. Maybe ConnectionIDFromBytes?

@hf
Copy link
Contributor Author

hf commented Nov 6, 2022

What is ReadConnectionID needed for?

I'm just re-exporting the methods from protocol. If it were up to me, that whole logic wouldn't live inside internal but that's what it is.

I am working on some experiments where I need greater control over connection IDs -- such as picking a different randomness generator (not crypto/rand).

ParseConnectionID should probably also have better name. Maybe ConnectionIDFromBytes?

Sure, tho I'd like to keep it similar to whatever is in protocol.

@marten-seemann
Copy link
Member

I am working on some experiments where I need greater control over connection IDs -- such as picking a different randomness generator (not crypto/rand).

Sure, that's what the ConnectionIDGenerator is there for. I still don't think we need both methods - just trying to keep the publicly exposed API clean (that's what internal is pretty good at ;))

ParseConnectionID should probably also have better name. Maybe ConnectionIDFromBytes?

Sure, tho I'd like to keep it similar to whatever is in protocol.

It's not really doing any parsing. Really, it's just converting from a byte slice, hence my suggestion.

@marten-seemann marten-seemann changed the title fix: allow custom connection id generation expose function to convert byte slice to a connection ID Nov 6, 2022
@marten-seemann marten-seemann linked an issue Nov 7, 2022 that may be closed by this pull request
@marten-seemann
Copy link
Member

@hf Are you planning to make these changes?

@hf
Copy link
Contributor Author

hf commented Nov 8, 2022

@hf Are you planning to make these changes?

Hey, yes I am but probably this weekend.

@hf hf force-pushed the hf/add-public-connection-id-generation branch from df8091f to 962204c Compare November 13, 2022 09:37
@hf
Copy link
Contributor Author

hf commented Nov 13, 2022

@marten-seemann Done!

@codecov
Copy link

codecov bot commented Nov 13, 2022

Codecov Report

Base: 85.48% // Head: 85.49% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (962204c) compared to base (df762b7).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3614      +/-   ##
==========================================
+ Coverage   85.48%   85.49%   +0.01%     
==========================================
  Files         141      142       +1     
  Lines       10296    10294       -2     
==========================================
- Hits         8801     8800       -1     
  Misses       1109     1109              
+ Partials      386      385       -1     
Impacted Files Coverage Δ
interface.go 0.00% <0.00%> (ø)
client.go 80.00% <0.00%> (+1.09%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@marten-seemann
Copy link
Member

Thank you!

@marten-seemann marten-seemann merged commit 7b211d6 into quic-go:master Nov 13, 2022
@hf hf deleted the hf/add-public-connection-id-generation branch November 15, 2022 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectionIDGenerator is not usable
2 participants