Skip to content

Conversation

@tegefaulkes
Copy link
Contributor

@tegefaulkes tegefaulkes commented Oct 27, 2023

Description

I need to expose more connectionId information so I compare connections better. This exposes the destination id as connectionIdPeer of the connection as well as a derived connectionIdShared.

Changes:

  • Exposed connection destination ID
  • Created a common ID that is identical for both sides of the connection. Derived from the source and destination ID.
  • The quiche connection is now the Source Of Truth for these IDs. The destinationId is only know after establishing and is only valid while running.
  • Added test demonstrating the new Ids and how they relate to each other.

As for logging, I'm switching non-internal connection and stream error logs to debug level. closing stream or connection with a non-0 code is not exceptional. Internal errors are.

Issues Fixed

Tasks

  1. expose more connectionId information on the QUICConnection.
  2. Reduce ERROR and WARN level logs for failing connections and streams. Application can make a decision to log these as errors or not.

Final checklist

  • Domain specific tests
  • Full tests
  • Updated inline-comment documentation
  • Lint fixed
  • Squash and rebased
  • Sanity check the final build

Changes:
* Exposed connection destination ID
* Created a common ID that is identical for both sides of the connection. Derived from the source and destination ID.
* The `quiche` connection is now the Source Of Truth for these IDs. The destinationId is only know after establishing and is only valid while running.
* Added test demonstrating the new Ids and how they relate to each other.

Related: MatrixAI/Polykey#609

[ci skip]
@tegefaulkes tegefaulkes self-assigned this Oct 27, 2023
@ghost
Copy link

ghost commented Oct 27, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@tegefaulkes
Copy link
Contributor Author

It made more sense to me to get the source ID and destination ID from quiche. It would be the best source of truth.

It's possible that these can change over the life of the connection and that would cause problems with QUICSocket logic. But that would only happen if the connection switches paths and I don't think we currently support that.

@tegefaulkes
Copy link
Contributor Author

I'd like the logging messages for connections to use a shared connection ID that is the same for both sides of the connection.

@CMCDragonkai
Copy link
Member

I don't think connection IDs can change. There is a connection migration thing but the ID is meant to be stable.

I'm wondering what is the main reason to expose these IDs and that the existing connection ID isn't sufficient?

@tegefaulkes
Copy link
Contributor Author

We only expose the source ID of the connection. But there are two IDs, the source and destination, these are reversed when comparing the two ends of the connection. So both sides have a different connectionId. So I can't actually tell what connections form pairs when logging because we only have half the information we need.

On top of that, I needed some kind of information that is shared between the two connections for making some decisions in MatrixAI/Polykey#592. So it's just good to have.

@tegefaulkes
Copy link
Contributor Author

I don't know the details, but I think connection IDs can change when the connection migrates.

Errors either bubble up or are handled at some point. Decision to log them out should be made there.

IMO we should rarely use `error` level logs because errors are either caught and handled and therefor not exceptional. Or bubble to the top in which case the program crashes.

[ci skip]
@CMCDragonkai
Copy link
Member

In QUIC as specified by RFC 9000, connection migration is a feature that allows the endpoints to change their IP address and/or port without terminating the existing connection. This is particularly useful for mobile clients that may switch between different networks, such as WiFi and cellular data.

The Connection ID (CID) is an identifier used in QUIC to distinguish different connections, and it can be used to route packets to the correct endpoint. The CID is especially useful in connection migration scenarios where the IP address and/or port may change. It provides a stable identifier that survives such changes, enabling uninterrupted communication.

In the context of connection migration in QUIC as per RFC 9000, changing the Connection ID is not a requirement. The specification does allow for endpoints to change the CID when migrating, but this is optional. If the CID does change, the endpoints need to inform each other using specific QUIC frames, such as NEW_CONNECTION_ID, to communicate the new CIDs.

So to directly answer your question: No, the Connection ID does not necessarily have to change when a QUIC connection is migrated, according to RFC 9000. It's optional and based on the strategies employed by the endpoints.

@tegefaulkes
Copy link
Contributor Author

https://docs.rs/quiche/latest/quiche/struct.Connection.html#method.source_id

Returns the source connection ID.

When there are multiple IDs, and if there is an active path, the ID used on that path is returned. Otherwise the oldest ID is returned.

Note that the value returned can change throughout the connection’s lifetime.

So according to quiche, the source and destination ID can change based on what path is being used. I don't know if we support paths. I don't think we do, so I don't think it's a problem. But it's something to be aware of.

@tegefaulkes
Copy link
Contributor Author

I've changed any QUICServer, QUICClient and QUICConnection error logs to info level and any in QUICStream to debug level.

@tegefaulkes tegefaulkes dismissed CMCDragonkai’s stale review October 27, 2023 05:59

It has been addressed.

@tegefaulkes tegefaulkes marked this pull request as ready for review October 27, 2023 05:59
@tegefaulkes tegefaulkes merged commit 55a5ec1 into staging Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants