Support Client Migration (Client Side) #4218

masa-koz · 2024-03-29T10:21:26Z

Description

This PR adds the support of Client Migration (Client SIde). Fix #1946.

Testing

I've added two test.

Basic/WithProbePathArgs.ProbePath/*
Basic/WithMigrationArgs.Migration/*

Documentation

In this PR, the following three Connection level Parameters will be added.

QUIC_PARAM_CONN_ADD_LOCAL_ADDRESS
QUIC_PARAM_CONN_REMOVE_LOCAL_ADDRESS
QUIC_PARAM_CONN_LOCAL_UNUSED_DEST_CID_COUNT

masa-koz · 2024-03-29T10:22:58Z

@microsoft-github-policy-service agree

nibanks · 2024-03-29T12:34:22Z

src/inc/msquic.h

@@ -909,6 +909,9 @@ typedef struct QUIC_SCHANNEL_CREDENTIAL_ATTRIBUTE_W {
 #define QUIC_PARAM_CONN_STATISTICS_V2                   0x05000016  // QUIC_STATISTICS_V2
 #define QUIC_PARAM_CONN_STATISTICS_V2_PLAT              0x05000017  // QUIC_STATISTICS_V2
 #define QUIC_PARAM_CONN_ORIG_DEST_CID                   0x05000018  // uint8_t[]
+#define QUIC_PARAM_CONN_ADD_LOCAL_ADDRESS               0x05000019  // QUIC_ADDR
+#define QUIC_PARAM_CONN_REMOVE_LOCAL_ADDRESS            0x0500001A  // QUIC_ADDR
+#define QUIC_PARAM_CONN_LOCAL_UNUSED_DEST_CID_COUNT     0x0500001B  // uint16_t


Is this for test only? Do real clients really need this? If it's test only, please put in msquicp.h.

Real clients also need this.

I'd really prefer to be able to abstract away the complexities of connection IDs from clients. Why exactly do clients need this information? To know if they can add a new path?

Yes. This information is needed to know when clients can add a new local address. But this is no longer needed if we adopt a new way in which clients defer sending a path challenge until they receive a new ConID.

src/inc/quic_platform.h

nibanks · 2024-03-29T12:35:48Z

src/platform/datapath_winkernel.c

@@ -2523,7 +2523,6 @@ CxPlatRecvDataReturn(
        DATAPATH_RX_IO_BLOCK* IoBlock =
            CXPLAT_CONTAINING_RECORD(Datagram, DATAPATH_RX_PACKET, Data)->IoBlock;

-        CXPLAT_DBG_ASSERT(Binding == NULL || Binding == IoBlock->Binding);


When a connection binds more than one bindings (i.e. after adding a new local address on the client side), we cannot assure that the Datagrams in RecvDataChain come from the same binding.

src/test/lib/PathTest.cpp

nibanks · 2024-03-29T12:42:15Z

src/test/lib/PathTest.cpp

+            QUIC_PARAM_CONN_STATISTICS_V2_PLAT,
+            &Size,
+            &Stats));
+    TEST_EQUAL(Stats.RecvDroppedPackets, 0);


How does this line help? Why would the client drop a packet? Also, are there other test conditions we can add here to determine if the new path worked?

The intention is that I would like to check whether the client succeeded in adding the source connection IDs to a newly opened bindings (if it failed, it will drop a packet whose destination is a newly added local address).
And I think that an event notification about the validation of a path is helpful for both testing and real application.

src/test/lib/PathTest.cpp

src/core/binding.h

src/core/path.h

nibanks · 2024-03-29T12:55:08Z

src/core/lookup.c

@@ -746,8 +768,24 @@ QuicLookupAddLocalCid(
            Hash);

    if (ExistingConnection == NULL) {
-        Result =
-            QuicLookupInsertLocalCid(Lookup, Hash, SourceCid, TRUE);
+        QUIC_CID_HASH_ENTRY* CID =


Just a thought, but we might need to start using pools for allocating and freeing all these entries so they don't hurt perf.

src/core/connection.c

nibanks · 2024-03-29T12:58:40Z

src/core/connection.c

+
+        BOOLEAN Collision = FALSE;
+
+        CxPlatDispatchLockAcquire(&MsQuicLib.DatapathLock);


Why do we have to grab a global lock here? That's not great, and definitely could cause perf issues.

I agree with you. I've tried to remove a global lock in masa-koz#5.
In this change, we will replace all the existing source connection IDs if we find the collision in adding a source connection ID to a newly opened bindings.

src/core/connection.c

nibanks · 2024-03-29T13:05:26Z

src/core/connection.c

+        if (!Connection->State.Started) {
+            Status = QUIC_STATUS_INVALID_STATE;
+            break;
+        }


Why not? It seems kind of intuitive to me that a client could create a connection, queue all paths it wants to use and then start it. No?

I agree with you. So, I've also implemented this. masa-koz#4

src/core/connection.c

nibanks · 2024-03-29T13:08:18Z

src/core/connection.c

+            break;
+        }
+
+        if (Connection->PathsCount == QUIC_MAX_PATH_COUNT) {


Put this check up before creating a new destination CID. Also, return 'invalid state' instead?

I agree with you. I will change this.

nibanks · 2024-03-29T13:08:49Z

src/core/connection.c

+
+        QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection);
+        if (NewDestCid == NULL) {
+            Status = QUIC_STATUS_INVALID_STATE;


I wonder if we can support a model where we always queue the path (somehow) even if we don't have a CID to use for it, yet...

I've implemented this.
https://github.com/masa-koz/msquic/pull/3/files

Thanks. The hardest part with this change overall will likely be that we need to make sure we have lots of tests for all the different scenarios.

nibanks · 2024-03-29T13:09:24Z

src/core/connection.c

+            break;
+        }
+
+        QUIC_CID_LIST_ENTRY* NewDestCid = QuicConnGetUnusedDestCid(Connection);


Does this need to be cleaned up at all the failure cases below?

I suppose that cleaning up is not needed because UsedLocally is not set yet. Isn't it?

src/core/connection.c

codecov · 2024-03-29T13:14:20Z

Codecov Report

Attention: Patch coverage is 76.29630% with 64 lines in your changes are missing coverage. Please review.

Project coverage is 84.20%. Comparing base (6ecf19b) to head (a339913).
Report is 5 commits behind head on main.

Files	Patch %	Lines
src/core/connection.c	67.42%	57 Missing ⚠️
src/core/binding.c	90.32%	3 Missing ⚠️
src/core/lookup.c	94.28%	2 Missing ⚠️
src/core/path.c	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4218      +/-   ##
==========================================
- Coverage   85.33%   84.20%   -1.14%     
==========================================
  Files          56       56              
  Lines       15358    15538     +180     
==========================================
- Hits        13105    13083      -22     
- Misses       2253     2455     +202

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

anrossi · 2024-04-16T23:08:32Z

src/core/binding.h

-    _In_ QUIC_BINDING* BindingSrc,
-    _In_ QUIC_BINDING* BindingDest,
-    _In_ QUIC_CONNECTION* Connection
+QuicBindingRemoveRemoteHash(


Does this mean there's no way to move source CIDs between bindings anymore?

No. We can move source CIDs between old binding and new binding by setting QUIC_PARAM_CONN_LOCAL_ADDRESS. In this case, we will call QuicBindingAddAllSourceConnectionIDs() for new binding and QuicBindingRemoveAllSourceConnectionIDs() for old binding.

anrossi · 2024-04-17T00:11:05Z

src/core/connection.c

+
+            QUIC_BINDING* Binding =
+                CXPLAT_CONTAINING_RECORD(Link, QUIC_BINDING, Link);
+            QUIC_CONNECTION* Connection1 =


Can we use a more descriptive name instead of Connection1? Like SearchConnection or CollisionConnection

I agree. I will change this.

anrossi · 2024-04-17T18:40:23Z

src/core/connection.c

+_IRQL_requires_max_(PASSIVE_LEVEL)
+static
+QUIC_STATUS
+QuicConnRemoveLocalAddress(


There doesn't appear to be any test coverage for the Remove address function here. Could you add a test for that?

anrossi · 2024-04-17T19:37:53Z

src/core/path.c

@@ -221,6 +243,7 @@ QuicConnGetPathForPacket(
                && QuicAddrGetFamily(&Packet->Route->RemoteAddress) == QuicAddrGetFamily(&Connection->Paths[i].Route.RemoteAddress)
                && QuicAddrCompareIp(&Packet->Route->RemoteAddress, &Connection->Paths[i].Route.RemoteAddress)
                && QuicAddrCompare(&Packet->Route->LocalAddress, &Connection->Paths[i].Route.LocalAddress)) {
+                QuicLibraryReleaseBinding(Connection->Paths[i].Binding);


Where is this reference added?

In the case of server, QuicLibraryTryAddRefBinding() was called in QuicConnGetPathForPacket(). In the case of client, QuicBindingGetLocalAddress() was called in QuicConnAddLocalAddress().
By the way, the non-null check should be added.

anrossi

First of all, your changes look great, thanks for doing this!
I'd like to see some more tests. Since you're adding new APIs, could you add some negative test cases for SetParam(QUIC_CONN_ADD_LOCAL_ADDRESS)/SetParam(QUIC_CONN_REMOVE_LOCAL_ADDRESS). For example, tests that add/remove an address that is not valid, add/remove an address before the connection is started, add the same address twice/remove the same address twice, a test that adds more addresses than the maximum path count, and one that adds an address that isn't on the system, but is valid, e.g. 100.64.10.20.

In the Path tests, I'd appreciate a test that adds an address and removes the old one and verifies no traffic drops. If you have time, also a test that removes all addresses from a connection.

Thanks again!

Add active migration support.

3ca34bb

masa-koz requested a review from a team as a code owner March 29, 2024 10:21