Add IPv6 support for host candidates #382

lherman-cs · 2020-04-23T19:34:14Z

Issue IPv6 Support #115
Description of changes:
- Add IPv6 support for host candidates
- Refractor iceAgentAddRemoteCandidate
Todo:
- Add IPv6 support for reflexive candidates
  - Verify the XOR mapped address for IPv6 implementation follows STUN RFC, https://tools.ietf.org/html/rfc5389#section-15.2 (Verify the XOR mapped address for IPv6 implementation follows STUN RFC #394)
- Add IPv6 support for TURN, https://tools.ietf.org/html/rfc6156
- Update getIpWithHostName to use getaddrinfo instead of gethostbyname since gethostbyname doesn't support IPv6 (Update getIpWithHostName to use getaddrinfo instead of gethostbyname #395)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-io · 2020-04-24T14:41:02Z

Codecov Report

Merging #382 into master will decrease coverage by 33.34%.
The diff coverage is 57.44%.

@@             Coverage Diff             @@
##           master     #382       +/-   ##
===========================================
- Coverage   87.07%   53.73%   -33.35%     
===========================================
  Files          33       33               
  Lines        8265     8267        +2     
===========================================
- Hits         7197     4442     -2755     
- Misses       1068     3825     +2757

Impacted Files	Coverage Δ
src/source/Ice/IceUtils.c	`95.49% <ø> (-1.43%)`	⬇️
src/source/Ice/IceAgent.c	`81.50% <56.81%> (-13.21%)`	⬇️
src/source/Ice/Network.c	`72.85% <66.66%> (-3.79%)`	⬇️
src/source/Ice/TurnConnection.c	`0.00% <0.00%> (-90.23%)`	⬇️
src/source/Signaling/ChannelInfo.c	`0.00% <0.00%> (-88.49%)`	⬇️
src/source/Signaling/LwsApiCalls.c	`1.55% <0.00%> (-83.02%)`	⬇️
src/source/Signaling/Signaling.c	`14.73% <0.00%> (-80.57%)`	⬇️
src/source/Rtp/Codecs/RtpVP8Payloader.c	`0.00% <0.00%> (-80.00%)`	⬇️
src/source/Signaling/StateMachine.c	`0.00% <0.00%> (-75.13%)`	⬇️
src/source/PeerConnection/Rtp.c	`30.40% <0.00%> (-43.25%)`	⬇️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a33d542...68ffd29. Read the comment docs.

MushMal

Nice work!

MushMal · 2020-04-27T21:42:29Z

src/source/Ice/IceAgent.c

@@ -256,12 +256,15 @@ STATUS iceAgentAddRemoteCandidate(PIceAgent pIceAgent, PCHAR pIceCandidateString
    PCHAR curr, tail, next;
    UINT32 tokenLen, portValue, remoteCandidateCount, i;
    BOOL foundIpAndPort = FALSE, freeIceCandidateIfFail = TRUE;
+    BOOL foundIp6 = FALSE;
+    CHAR ip6Buf[KVS_MAX_IPV6_ADDRESS_STRING_LEN+1];


Might need to setup some editor auto-formatting preferences. In this case, the '+' sign should have spaces around it

can use KVS_IP_ADDRESS_STRING_BUFFER_LEN instead

Do we have a standard format config? Something like .clang-format?

Ah, didn't realize that we have KVS_IP_ADDRESS_STRING_BUFFER_LEN. I'll update it. Thanks.

MushMal · 2020-04-27T21:43:35Z

src/source/Ice/IceAgent.c

    KvsIpAddress candidateIpAddr;

    CHK(pIceAgent != NULL && pIceCandidateString != NULL, STATUS_NULL_ARG);
    CHK(!IS_EMPTY_STRING(pIceCandidateString), STATUS_INVALID_ARG);

    MEMSET(&candidateIpAddr, 0x00, SIZEOF(KvsIpAddress));
+    MEMSET(ip6Buf, 0x00, KVS_MAX_IPV6_ADDRESS_STRING_LEN+1);


ditto. I won't add comments about the formatting.

use SIZEOF(ip6Buf) ?

Yep, makes sense.

MushMal · 2020-04-27T21:46:22Z

src/source/Ice/IceAgent.c

        } else if (STRNCHR(curr, tokenLen, '.') != NULL) {
            CHK(tokenLen <= KVS_MAX_IPV4_ADDRESS_STRING_LEN, STATUS_ICE_CANDIDATE_STRING_INVALID_IP); // IPv4 is 15 characters at most
            CHK_STATUS(populateIpFromString(&candidateIpAddr, curr));
+        } else {
+            STRNCPY(ip6Buf, curr, KVS_MAX_IPV6_ADDRESS_STRING_LEN);
+            foundIp6 = inet_pton(AF_INET6, ip6Buf, candidateIpAddr.address) == 1 ? TRUE : FALSE;


This is actually Windows supported.

MushMal · 2020-04-27T21:53:06Z

src/source/Ice/IceAgent.c

        }

        curr = next + 1;
-        foundIpAndPort = (candidateIpAddr.port != 0) && (candidateIpAddr.address[0] != 0);
+        foundIpAndPort = (candidateIpAddr.port != 0) && ((candidateIpAddr.address[0] != 0) || foundIp6);


Perhaps I am not following the logic correctly. KvsIpAddress can hold both IPv4 and IPv6 addresses. The inners of the while loop should set this param and you can check this param only. Also, consult with Felix, I am not sure why the check is against 0s. Can't the first byte of the address be 0 and the port be 0? I think an explicit 'found' boolean should be a good sentinel rather than checking for zeroed values.

I've discussed this offline with @chehefen. He didn't write this part. But, we agreed that it makes more sense to have a boolean to check this instead. So, I'll update the PR to use a boolean instead.

MushMal · 2020-04-27T21:54:58Z

src/source/Ice/IceAgent.c

@@ -1047,6 +1055,9 @@ STATUS iceAgentSendSrflxCandidateRequest(PIceAgent pIceAgent)
            switch(pCandidate->iceCandidateType) {
                case ICE_CANDIDATE_TYPE_SERVER_REFLEXIVE:
                    pIceServer = &(pIceAgent->iceServers[pCandidate->iceServerIndex]);
+                    if (pIceServer->ipAddress.family != pCandidate->ipAddress.family) {
+                      break;


We are trying to minimize the usage of the control flow keywords such as break and continue. It's not a good coding style to use break within a switch all together. Use the negation of the boolean predicate here

if (family == other) {
// do the stuff
}

Got it. I'll update it.

MushMal · 2020-04-27T21:55:24Z

src/source/Ice/IceAgent.c

@@ -1324,7 +1335,9 @@ STATUS iceAgentInitSrflxCandidate(PIceAgent pIceAgent)
        pCurNode = pCurNode->pNext;
        pCandidate = (PIceCandidate) data;

-        if (pCandidate->iceCandidateType == ICE_CANDIDATE_TYPE_HOST) {
+        // TODO: Stop skipping IPv6. Stun serialization and deserialization needs to be implemented properly first.


Add a GitHub issue tracking this work

MushMal · 2020-04-27T21:56:03Z

src/source/Ice/IceAgent.c

@@ -1402,8 +1415,15 @@ STATUS iceAgentInitRelayCandidate(PIceAgent pIceAgent)
            pSocketAddrForTurn = NULL;
            // if an VPN interface (isPointToPoint is TRUE) is found, use that instead
            for(i = 0; i < pIceAgent->localNetworkInterfaceCount; ++i) {
-                if(pIceAgent->localNetworkInterfaces[i].family == pIceAgent->iceServers[j].ipAddress.family &&
-                   (pSocketAddrForTurn == NULL || pIceAgent->localNetworkInterfaces[i].isPointToPoint)) {
+                // TODO: Stop skipping IPv6. Stun serialization and deserialization needs to be implemented properly first. 


Same comment about GitHub issue tracking it

Same comment as above.

lherman-cs force-pushed the ipv6 branch 7 times, most recently from e6aa068 to 1fcb008 Compare April 24, 2020 14:40

lherman-cs force-pushed the ipv6 branch from 1fcb008 to 5a4e668 Compare April 24, 2020 16:06

lherman-cs marked this pull request as ready for review April 24, 2020 16:22

lherman-cs requested review from Sean-Der, chehefen and MushMal April 24, 2020 16:22

lherman-cs force-pushed the ipv6 branch 7 times, most recently from 0b2c49a to 83b4492 Compare April 27, 2020 19:41

MushMal reviewed Apr 27, 2020

View reviewed changes

lherman-cs added 2 commits April 27, 2020 21:20

Enable IPv6 support in Dockers to workaround travis-ci/travis-ci#8891

01c5857

Add IPv6 support for host candidates

cc8c6b2

lherman-cs force-pushed the ipv6 branch from 83b4492 to 128d115 Compare April 28, 2020 01:40

Refractor iceAgentAddRemoteCandidate

68ffd29

lherman-cs force-pushed the ipv6 branch from 128d115 to 68ffd29 Compare April 28, 2020 01:51

MushMal approved these changes Apr 28, 2020

View reviewed changes

lherman-cs merged commit db511e1 into awslabs:master Apr 28, 2020

lherman-cs deleted the ipv6 branch April 28, 2020 12:42

lherman-cs mentioned this pull request May 1, 2020

IPv6 Support #115

Closed

ycyang1229 mentioned this pull request Jun 23, 2020

[QUESTION] The support of IPv6 #561

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add IPv6 support for host candidates #382

Add IPv6 support for host candidates #382

lherman-cs commented Apr 23, 2020 •

edited

Loading

codecov-io commented Apr 24, 2020 •

edited

Loading

MushMal left a comment

MushMal Apr 27, 2020

chehefen Apr 27, 2020

lherman-cs Apr 27, 2020

MushMal Apr 27, 2020

chehefen Apr 27, 2020

lherman-cs Apr 27, 2020

MushMal Apr 27, 2020

MushMal Apr 27, 2020

lherman-cs Apr 27, 2020

MushMal Apr 27, 2020

lherman-cs Apr 27, 2020

MushMal Apr 27, 2020

lherman-cs Apr 28, 2020

MushMal Apr 27, 2020

lherman-cs Apr 28, 2020

Add IPv6 support for host candidates #382

Add IPv6 support for host candidates #382

Conversation

lherman-cs commented Apr 23, 2020 • edited Loading

codecov-io commented Apr 24, 2020 • edited Loading

Codecov Report

MushMal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lherman-cs commented Apr 23, 2020 •

edited

Loading

codecov-io commented Apr 24, 2020 •

edited

Loading