add support for providing a custom Connection ID generator via Config

[joliveirinha]

This work makes it possible for servers or clients to control how
ConnectionIDs are generated, which in turn will force peers in the
connection to use those ConnectionIDs as destination connection IDs when sending packets.

This is useful for scenarios where we want to perform some kind
selection on the QUIC packets at the L4 level.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[marten-seemann]

Is this PR sufficient so that one could implement QUIC-LB using this config option?

[codecov]

Codecov Report

Base: 85.69% // Head: 85.57% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (ddee101) compared to base (8c0c481).
Patch coverage: 86.36% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3452      +/-   ##
==========================================
- Coverage   85.69%   85.57%   -0.12%     
==========================================
  Files         136      137       +1     
  Lines        9992    10016      +24     
==========================================
+ Hits         8562     8571       +9     
- Misses       1052     1067      +15     
  Partials      378      378              

Impacted Files	Coverage Δ
connection.go	77.64% <ø> (-0.02%)	⬇️
internal/protocol/connection_id.go	75.00% <0.00%> (-6.82%)	⬇️
server.go	81.10% <50.00%> (+0.51%)	⬆️
client.go	78.91% <100.00%> (ø)
config.go	100.00% <100.00%> (ø)
conn_id_generator.go	93.10% <100.00%> (+3.82%)	⬆️
closed_conn.go	64.71% <0.00%> (-14.71%)	⬇️
packet_handler_map.go	72.37% <0.00%> (-1.73%)	⬇️
http3/server.go	73.04% <0.00%> (-1.49%)	⬇️
... and 6 more

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.

[joliveirinha]

Is this PR sufficient so that one could implement QUIC-LB using this config option?

This PR doesn't make any assumption on the scheme used for the ConnectionIDs, so yes, from my understanding of that draft, one could provide a custom generator to the Config that follows the QUIC LB connection ID scheme.

[joliveirinha]

I have updated the PR with more doc and the suggested improvements.
The build for go1.18 failed but seems a flake. I am unable to re-run, tho

[joliveirinha]

@marten-seemann is this good to go, or do you want to address anything extra?

[marten-seemann]

Sorry for the long delay!

If I understand correctly, a ConnectionIDGenerator needs to generate CIDs with constant length, right? It's not possible to encode the length into the first few bits of the CID, and then parse it when receiving a packet? If that's a requirement, this should probably be documented on the interface.

This is useful for scenarios where we want to perform some kind selection on the QUIC packets at the L4 level.

Just to help me understand, can you explain a bit more about your use case?

[joliveirinha]

If I understand correctly, a ConnectionIDGenerator needs to generate CIDs with constant length, right?
Yes. In this case I did that because the whole code already had that assumption. I didn't changed that assumption, and just made it in the interface.
To be honest, I don't know if RFC states it is possible for QUIC connections to re-issue CID with different lengths during their lifetime. And if it does, what would the benefit be at this point.

So, in my opinion, unless there is a valid use-case, making that change would require changing other parts of the code unrelated to this feature. At least as far as my understanding of the code goes.

It's not possible to encode the length into the first few bits of the CID, and then parse it when receiving a packet?
That would be possible yes, but that is already an implementation specification of the exact format.
here, this approach allows implementations of quic-lb or others that may not encode that info.

For instance, in our use-case, our format doesn't do that.
It's very similar to QUIC-LB, in the sense it encodes server information but doesn't follow exactly that format.

If that's a requirement, this should probably be documented on the interface.
yes. can do that.

Just to help me understand, can you explain a bit more about your use case?
sure.

Basically we have a datacenter with servers using the same IP (anycast). So connections from clients have as dstIP the same anycast IP.
In front of the DC we have routes that use ECMP. These route packets based on the 5-tuple hash of the packet and the pool of servers of the DC.
If the pool of servers change, then packets will be diverted to a different server and connection broken. So we need something that is smarter and looks into the QUIC DCID and routes packets to the right server.
That is why we need this.

Le me know if you need more details. If not, I can add the extra documentation on the connection id len assumption.

[marten-seemann]

Thank you for your explanation, that makes a lot of sense to me.

So, in my opinion, unless there is a valid use-case, making that change would require changing other parts of the code unrelated to this feature. At least as far as my understanding of the code goes.

Indeed, one would need to call the generator when parsing packet headers. I'm ok with not implementing this (we can still add it later, when somebody makes a compelling case), as long as we're really clear about the API contract of the generator. So yes, a clarification of the documentation there would be appreciated.

[joliveirinha]

Thanks. Done, updated doc to be more clear. Also, rebased with master.

[marten-seemann]

@joliveirinha The linter is unhappy. Can you please fix?

[marten-seemann]

I was trying to add an integration test to this PR, but I don't permissions to push to the branch. Maybe you could apply this commit: 6522274?

[joliveirinha]

@marten-seemann done.

[marten-seemann]

@joliveirinha The linter is still unhappy with the PR.

[joliveirinha]

you are right. missed that! should be fine now.

[marten-seemann]

@joliveirinha The linter is still unhappy :(
I know this is annoying. Thank you for your patience and for helping push this PR over the finish line!

[joliveirinha]

No problem. I think now it is ok :)

maybe we should have a makefile that runs these tests locally.

[marten-seemann]

maybe we should have a makefile that runs these tests locally.

Unfortunately, it's not that easy. The CI config specifies a certain version of golangci-lint, and test failure will depend on that. Would be great if there was a way to run a GitHub Actions workflow locally, but as far as I know GitHub doesn't provide an easy way to do that.