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

[+] Explicit path identifier first shot #292

Merged
merged 61 commits into from
Mar 25, 2024
Merged

[+] Explicit path identifier first shot #292

merged 61 commits into from
Mar 25, 2024

Conversation

Yanmei-Liu
Copy link
Contributor

As the design required revision of the entire draft, this PR attempts to be a start point.

Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Yanmei-Liu! I added some suggestions.

The biggest open question I have is what happens to the path identifier when peers have reached max_concurrent_paths. In your proposal, it sounds like path identifiers are reused. This might have weird corner cases when packets are delayed.
The other way to implement this is to have path identifiers increment. We need to have a consistent view of how many paths are currently in use between the two peers.

for its peer. When endpoints address a path in multipath control frames,
it refers to the Path Identifier field of the destination Connection ID
used for sending packets on that particular path. Note that the Path Identifier
for initial path is always 0.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should include a precise definition of what we mean by "path". Crucially, a "path" is not defined by the 5-tuple, since NAT rebindings change the 5-tuple without one of the peers knowing about it.

Instead, a new path is opened when a peer intentionally starts sending packets from a new address.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am implementing this right now. The good thing about the "unique path ID" is that the header now carries a clear intention. If the destination CID correspond to path X, then the intention was clearly to send the packet on path X -- even in cases when the CID is not exactly the same that was used for that path previously. This closes the ambiguity of "CID renewal and NAT Rebinding at the same time".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record in issue #291

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
@Yanmei-Liu
Copy link
Contributor Author

The biggest open question I have is what happens to the path identifier when peers have reached max_concurrent_paths. In your proposal, it sounds like path identifiers are reused. This might have weird corner cases when packets are delayed. The other way to implement this is to have path identifiers increment. We need to have a consistent view of how many paths are currently in use between the two peers.

That's an important point that we need to make it clear. I think it's better to reuse than increment, as we can add some limitation for the corner cases(e.g. Path Identifier can only be reused after 3 max PTOs after the path is abandoned and all the CIDs belonging to this path is retired). It's more friendly for resource limitation of endpoints.

If we use increment, then we need to discuss when to increase the max Path Identifier in the new connection IDs without breaking the limitation for max concurrent paths.

Copy link
Contributor

@qdeconinck qdeconinck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really happy to see such a proposal as a PR. Here and there comments and suggestions.

I think such a proposal also opens the door for a more explicit and controlled path management. While we could have up to max_active_paths paths, an endpoint could only enable its peer to open them when needed (e.g., by proposing a new Path ID when it thinks it would benefit from a new path). We could also revisit the notion of "path abandon" and make this a strong signal (i.e., retire the Path ID) instead of a recommendation (i.e., please remove associated CIDs and stop using the path).

draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
draft-ietf-quic-multipath.md Outdated Show resolved Hide resolved
Copy link
Contributor

@huitema huitema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this work. I think three key points need to be fixed:

  • the CID should be uniquely identified by their sequence number, not by the combination of path-id and sequence number.
  • we do not need MP_RETIRE_CID
  • we should never reuse path-ID, because they are part of the cryptographic nonce.

draft-ietf-quic-multipath.md Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
draft-ietf-quic-multipath.md Show resolved Hide resolved
Yanmei-Liu and others added 2 commits November 21, 2023 10:51
Co-authored-by: Quentin De Coninck <quentin.deconinck@umons.ac.be>
Co-authored-by: Quentin De Coninck <quentin.deconinck@umons.ac.be>
Yanmei-Liu and others added 5 commits March 12, 2024 23:21
Co-authored-by: mirjak <mirja.kuehlewind@ericsson.com>
Co-authored-by: mirjak <mirja.kuehlewind@ericsson.com>
Co-authored-by: mirjak <mirja.kuehlewind@ericsson.com>
a zero-length value.

If any of the endpoints does not advertise the enable_multipath transport
- initial_max_paths (current version uses 0x0f739bbc1b666d07): the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better: initial_max_path_id. This would mirror the meaning of the frame as suggested in https://github.com/quicwg/multipath/pull/292/files#r1527500161.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should remain initial_max_paths to be symmetric with initial_max_streams_*.

The explanation can be copied from RFC 9000:

Setting this parameter is equivalent to sending a MAX_PATHS of the corresponding type with the same value.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My 2c: There's only so much value you get from consistency with RFC 9000. This has created a fair amount of confusion in the review of this PR, and it seems like calling it initial_max_path_id (and the corresponding renaming of the frame), would make it 100% clear what is meant, without the need for long explanations.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then I think that the frame type should be MAX_PATH_ID to further reduce the confusion and increase the multipath specification consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record in issue #324

of the associated Path Identifier. When endpoint received PATH_ABANDON frame,
it SHOULD not use the associated Path Identifier in future packets, it can
only use the Path ID in ACK_MP frames for inflight packets or
in MP_RETIRE_CONNECTION_ID frames for CID retirement.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should PATH_ABANDON automatically retire the connection IDs, without the need to send any frames? It would be nice if we don't have to deal with a malicious / misbehaving peer that abandons a path, but keeps the CIDs active.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it should! Or at least the complete closing of a path, with dual PATH_ABANDON frames should.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record in #313

Comment on lines +543 to +545
Each endpoints pre-allocate a Path Identifier for each new Connection ID.
The Path Identifier 0 indicates the initial path of the connection.
Endpoints SHOULD issue at least one unused Connection ID with unused Path Identifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems backwards. Connection IDs are issues for paths, not the other way around.
Also, if I allow you to open 1 million paths, you're probably wise to not issue me 1 million CIDs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Record in #325

Yanmei-Liu and others added 6 commits March 17, 2024 21:22
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
Co-authored-by: Marten Seemann <martenseemann@gmail.com>
[+] Explicit Path ID: solving issue #297
@Yanmei-Liu Yanmei-Liu merged commit da86c1e into main Mar 25, 2024
1 of 3 checks passed
@mirjak mirjak deleted the dev/path_id branch May 28, 2024 12:15
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.

separate Path IDs from Connection IDs