Fix Uri.Host for IPv6 Link-local address #29829

MarcoRossignoli · 2018-05-21T19:05:40Z

contributes to #28863
replace PR #29769 after revert #29818 for Outerloop CI fail

this PR address the second part of issue:

Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part.

Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18].
Note: Uri.IdnHost correctly contains the %number part.

cc: @rmkerr @Caesar1995 @davidsh @stephentoub

MarcoRossignoli · 2018-05-21T19:06:21Z

@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

davidsh · 2018-05-21T19:10:13Z

src/System.Private.Uri/src/System/Uri.cs

@@ -2616,6 +2618,10 @@ private static string CreateHostStringHelper(string str, ushort idx, ushort end,
            {
                flags |= Flags.LoopbackHost;
            }
+            if(linkLocalAddress)


add space after 'if'

davidsh · 2018-05-21T19:10:23Z

src/System.Private.Uri/src/System/Uri.cs

+            if(linkLocalAddress)
+            {
+                flags |= Flags.LinkLocalAddress;
+            }


add newline after closing brace

davidsh · 2018-05-21T19:11:16Z

src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs

+        [Fact]        
+        public void IdnDnsSafeHost_IPv6HostLinkLocalAddress_ScopeIdButNoBrackets()
+        {
+            Uri test = new Uri("http://[fe80::e077:c9a3:eeba:b8e9%18]/");


You should refactor this duplicated string, "fe80::e077:c9a3:eeba:b8e9%18" into a constant in this test.

i copied the pattern from test above https://github.com/MarcoRossignoli/corefx/blob/2d9eddafdf568afab7a8bb730415bfce9b885a15/src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs#L41 do you confirm this refactoring?

i copied the pattern from test above https://github.com/MarcoRossignoli/corefx/blob/2d9eddafdf568afab7a8bb730415bfce9b885a15/src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs#L41 do you confirm this refactoring?

Following existing code and test patterns can be useful. But it doesn't work in all cases. Sometimes there are bad patterns in the code. And we don't change them all, but try to add better code for new PRs. Also, the pattern you cited uses simpler IP literals. This test uses a long IPv6 literal. So, for code cleanliness and ease of maintenance, it's better to consolidate if possible.

done with [Theory] could be useful for future new data, if you like.

MarcoRossignoli · 2018-05-21T19:27:17Z

@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

davidsh · 2018-05-21T21:36:39Z

src/System.Private.Uri/src/System/Uri.cs

@@ -2617,11 +2617,12 @@ private static string CreateHostStringHelper(string str, ushort idx, ushort end,
            if (loopback)
            {
                flags |= Flags.LoopbackHost;
-            }
-            if(linkLocalAddress)
+            }            


There seems to be extra whitespace (space or tabs) after this closing brace.

MarcoRossignoli · 2018-05-22T12:39:22Z

@dotnet-bot test Outerloop Linux x64 Release Build please, on my Ubuntu 18.04 outerloop works well, i want to understand if the issue is related to SO version.

MarcoRossignoli · 2018-05-22T13:33:17Z

@dotnet-bot test Linux x64 Release Build please

MarcoRossignoli · 2018-05-22T14:31:01Z

@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

stephentoub · 2018-05-22T15:49:01Z

GetAsync_IPv6LinkLocalAddressUri_Success is failing with this change with CurlHandler on several distros:
https://mc.dot.net/#/user/MarcoRossignoli/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/36d25f6abe1a87b6fd0fb9fdea3cde817752397f/workItem/System.Net.Http.Functional.Tests/analysis/xunit/System.Net.Http.Functional.Tests.PlatformHandler_HttpClientHandlerTest~2FGetAsync_IPv6LinkLocalAddressUri_Success

MarcoRossignoli · 2018-05-22T15:54:13Z

GetAsync_IPv6LinkLocalAddressUri_Success is failing with this change with CurlHandler on several distros:

@stephentoub i need to setup an Ubuntu 14.04, i think depends on SO version(on my 18.04 it's ok, also on CI), let me try!

EDIT(22/5): i repro issue with Ubuntu 14.04, i think depends on curl version, i'm checking fix/releases

caesar-chen · 2018-05-22T22:51:31Z

@dotnet-bot test Windows x64 Debug Build

MarcoRossignoli · 2018-05-23T20:43:19Z

UPDATE: after some research i discovered that there is a difference in ipv6 parsing between curl versions, this PR change the way of parsing Fixed in 7.37.0 - May 21 2014 Ubuntu 14.04 use curl ver 7.35.0(curl -V) now single % is supported https://github.com/curl/curl/blob/master/lib/url.c#L2358.
Now to be backward compatible my idea is to amend here https://github.com/dotnet/corefx/blob/master/src/System.Net.Http/src/System/Net/Http/CurlHandler/CurlHandler.EasyRequest.cs#L266 and in case of LLA reassemble an Url with %25 instead of %. What do you think?I did some test with code and curl command and seem to be working(i didn't to all tests outerloop/distros), but before moving on i'd like to hear your opinions.

cc @stephentoub @davidsh @rmkerr @Caesar1995

MarcoRossignoli · 2018-05-24T12:05:08Z

@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

MarcoRossignoli · 2018-05-24T13:15:53Z

src/System.Net.Http/src/System/Net/Http/CurlHandler/CurlHandler.EasyRequest.cs

-                string url = requestUri.Host == idnHost ? 
-                                requestUri.AbsoluteUri : 
-                                new UriBuilder(requestUri) { Host = idnHost }.Uri.AbsoluteUri;
+                string url = requestUri.Host == idnHost ?


Another idea could be borrow some code from here or here and get only host name value, if in future we fix brackets on idnHost this could break, but i do not know if it is worth.

As long as the tests would catch that break, I think the change is probably okay. The fix to add brackets on idnHost is risky enough that it will require more investigation.

stephentoub · 2018-05-24T15:07:26Z

src/System.Net.Http/src/System/Net/Http/CurlHandler/CurlHandler.EasyRequest.cs

@@ -263,7 +263,8 @@ private void SetUrl()
                Uri requestUri = _requestMessage.RequestUri;

                long scopeId;
-                if (IsLinkLocal(requestUri, out scopeId))
+                bool isLinkLocal = false;
+                 if ((isLinkLocal = IsLinkLocal(requestUri, out scopeId)))


Nit: indentation is off

stephentoub · 2018-05-24T15:07:58Z

src/System.Net.Http/src/System/Net/Http/CurlHandler/CurlHandler.EasyRequest.cs

-                                new UriBuilder(requestUri) { Host = idnHost }.Uri.AbsoluteUri;
+                string url = requestUri.Host == idnHost ?
+                             requestUri.AbsoluteUri :
+                             new UriBuilder(requestUri){ Host = isLinkLocal && (scopeIdIndex = idnHost.IndexOf("%", StringComparison.OrdinalIgnoreCase)) != -1 ? idnHost.Substring(0, scopeIdIndex) : idnHost}.Uri.AbsoluteUri;


The IndexOf can just be idnHost.IndexOf('%')

you're right

stephentoub · 2018-05-24T15:10:58Z

src/System.Private.Uri/src/System/IPv6AddressHelper.cs

@@ -31,7 +31,8 @@ internal static string ParseCanonicalName(string str, int start, ref bool isLoop
                ((long*)numbers)[0] = 0L;
                ((long*)numbers)[1] = 0L;
                isLoopback = Parse(str, numbers, start, ref scopeId);
-                return '[' + CreateCanonicalName(numbers) + ']';
+                linkLocalAddress = numbers[0] == 0xfe80;
+                return '[' + CreateCanonicalName(numbers) + (linkLocalAddress ? scopeId : "") + ']';


Nit: the compiler will help fix this up, but it'd be better to use "[" and "]" rather than '[' and ']'.

enlighten me...why?

Because we want this to compile down to the efficient string.Concat(string, string, string, string) call. If the compiler didn't help here, this would end up being a call to string.Concat(params object[] args), which would result in an object[] allocation, the chars getting boxed (so two more allocations), and then having ToString called on them (so two more allocations), plus having a string array allocated in the Concat implementation to store the ToString on each input. As of dotnet/roslyn#415, the C# compiler should do the conversion from e.g. '[' to "[" for you, but why make it.

understood TYVM!

davidsh · 2018-05-24T20:04:54Z

src/System.Private.Uri/src/System/Uri.cs

@@ -114,6 +114,7 @@ private enum Flags : ulong
            FragmentIriCanonical = 0x40000000000,
            IriCanonical = 0x78000000000,
            UnixPath = 0x100000000000,
+            LinkLocalAddress = 0x200000000000


"LinkLocalAddress -> "IPv6LinkLocalAddress"

davidsh · 2018-05-24T20:05:18Z

src/System.Private.Uri/src/System/Uri.cs

@@ -2616,6 +2618,11 @@ private static string CreateHostStringHelper(string str, ushort idx, ushort end,
            {
                flags |= Flags.LoopbackHost;
            }
+            if (linkLocalAddress)


add newline above this 'if' statement.

davidsh · 2018-05-24T20:08:43Z

src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs

+        [Theory]
+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
+        [InlineData("fe80::e077:c9a3:eeba:b8e9", "%18")]
+        public void IdnDnsSafeHost_IPv6HostLinkLocalAddress_ScopeIdButNoBrackets(string address, string zoneIndex)


"ScopeIdButNoBrackets" part of the test name isn't quite right since there is an Assert in this test that validates for brackets.

Normally we don't have a single test method that test multiple properties. But I see that there is already other tests that do this in this file.

I suggest, though, that you rename the test like this:

"IdnDnsSafeHost_IPv6HostLinkLocalAddress_ScopeIdCorrectlyFormatted"

davidsh · 2018-05-24T20:10:46Z

src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

+        [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
+        public void Host_IPv6LinkLocalAddress_HasScopeId(string address, string zoneIndex)
+        {
+            string scopedLiteralIpv6 = "[" + address + zoneIndex + "]";


"scopedLiteralIpv6" -> "scopedLiteralIpv6Brackets" to be more consistent with variable naming in the tests above.

davidsh · 2018-05-24T20:10:55Z

src/System.Private.Uri/tests/FunctionalTests/UriIpHostTest.cs

+        [InlineData("fe81::e077:c9a3:eeba:b8e9", "%18")]
+        public void Host_NonIPv6LinkLocalAddress_NoScopeId(string address, string zoneIndex)
+        {
+            string scopedLiteralIpv6 = "[" + address + zoneIndex + "]";


"scopedLiteralIpv6" -> "scopedLiteralIpv6Brackets" to be more consistent with variable naming in the tests above.

MarcoRossignoli · 2018-05-24T20:44:51Z

@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

caesar-chen · 2018-05-25T04:54:38Z

src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs

@@ -45,6 +45,21 @@ public void IdnDnsSafeHost_IPv6Host_ScopeIdButNoBrackets()
            Assert.Equal("[::1]", test.Host);
        }

+        [Theory]


We usually use [Theory] for multiple test data. So here

you can add more test data ([InlineData])

or use [Fact], without taking any parameters, and create a constant in the test. As David mentioned here.

caesar-chen · 2018-05-25T04:56:23Z

@dotnet-bot test this please

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build

MarcoRossignoli · 2018-05-25T08:07:19Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

caesar-chen · 2018-05-25T16:53:16Z

src/System.Private.Uri/tests/FunctionalTests/IdnDnsSafeHostTest.cs

        {
+            string address = "fe80::e077:c9a3:eeba:b8e9";
+            string zoneIndex = "%18";
+


Use

const string ScopedLiteralIpv6 = "fe80::e077:c9a3:eeba:b8e9%18";

since address and zoneIndex is not used anywhere else in the test.

oh yes i missed it sorry

done, updated also other test with constants.

MarcoRossignoli · 2018-05-25T17:13:43Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

caesar-chen · 2018-05-25T17:21:37Z

LGTM once CI is green.

MarcoRossignoli · 2018-05-25T20:48:15Z

@dotnet-bot test Outerloop Windows x64 Debug Build

MarcoRossignoli · 2018-05-26T07:28:07Z

@dotnet-bot test Outerloop Windows x64 Debug Build

caesar-chen

LGTM.
@dotnet/ncl Any additional feedback?

caesar-chen · 2018-05-28T20:11:06Z

src/System.Private.Uri/src/System/Uri.cs

@@ -1193,7 +1194,7 @@ public string DnsSafeHost
                if (HostType == Flags.IPv6HostType)
                {
                    ret = ret.Substring(1, ret.Length - 2);
-                    if ((object)_info.ScopeId != null)
+                    if ((object)_info.ScopeId != null && (_flags & Flags.IPv6LinkLocalAddress) == 0)


Nit: We can add a comment here to explain the purpose for the new flag. i,e,. to avoid duplicated ScopeId.

Added, with new line before for readability, let me know if it's ok.

MarcoRossignoli · 2018-05-28T20:55:03Z

@dotnet-bot test Outerloop Windows x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Debug Build
@dotnet-bot test Outerloop NETFX x86 Debug Build
@dotnet-bot test Outerloop UWP CoreCLR x64 Debug Build
@dotnet-bot test Outerloop Linux x64 Release Build please
@dotnet-bot test Outerloop Windows x86 Release Build please

rmkerr

This looks like a solid, well scoped change to me. It's unfortunate that we have to special case the behavior on Curl, but I think that the change you made there is correct. Pending approval from either @davidsh or @stephentoub, this looks good to me.

This reverts commit f7a8cf1.

Contributes to dotnet/corefx#28863 Replace PR dotnet/corefx#29769 after revert dotnet/corefx#29818 for Outerloop CI fail This PR address the second part of issue: Uri.Host LLA (Link-local address) IPv6 address doesn't contain %number part. Currently it returns [fe80::e077:c9a3:eeba:b8e9], it should return [fe80::e077:c9a3:eeba:b8e9%18]. Note: Uri.IdnHost correctly contains the %number part. Commit migrated from dotnet/corefx@f7a8cf1

…" (dotnet/corefx#30062) This reverts commit dotnet/corefx@f7a8cf1. Commit migrated from dotnet/corefx@e5bb0b5

fix

f2ded59

davidsh requested review from stephentoub and a team May 21, 2018 19:09

davidsh added the area-System.Net label May 21, 2018

davidsh reviewed May 21, 2018

View reviewed changes

MarcoRossignoli added 2 commits May 21, 2018 21:17

skip test on netfx

2d9edda

address PR feedback

64f3c78

davidsh reviewed May 21, 2018

View reviewed changes

address PR feedbacl

36d25f6

MarcoRossignoli changed the title ~~Fix Uri.Host for IPv6 Link-local address~~ [WIP]Fix Uri.Host for IPv6 Link-local address May 22, 2018

karelz assigned MarcoRossignoli and rmkerr May 22, 2018

CurlHandler fix attempt

b565053

MarcoRossignoli commented May 24, 2018

View reviewed changes

MarcoRossignoli changed the title ~~[WIP]Fix Uri.Host for IPv6 Link-local address~~ Fix Uri.Host for IPv6 Link-local address May 24, 2018

stephentoub reviewed May 24, 2018

View reviewed changes

davidsh reviewed May 24, 2018

View reviewed changes

davidsh added this to the 2.2.0 milestone May 24, 2018

address PR feedback

0ed1e60

caesar-chen reviewed May 25, 2018

View reviewed changes

address PR feedback

e729a15

caesar-chen reviewed May 25, 2018

View reviewed changes

address PR feedback

192781b

caesar-chen approved these changes May 28, 2018

View reviewed changes

address PR feedback

4517b55

rmkerr approved these changes May 29, 2018

View reviewed changes

davidsh approved these changes May 30, 2018

View reviewed changes

davidsh merged commit f7a8cf1 into dotnet:master May 30, 2018

MarcoRossignoli deleted the ipv6lla branch May 30, 2018 18:10

caesar-chen added a commit to caesar-chen/corefx that referenced this pull request Jun 1, 2018

Revert "Fix Uri.Host for IPv6 Link-local address (dotnet#29829)"

019f18b

This reverts commit f7a8cf1.

caesar-chen pushed a commit that referenced this pull request Jun 4, 2018

Revert "Fix Uri.Host for IPv6 Link-local address (#29829)" (#30062)

e5bb0b5

This reverts commit f7a8cf1.

caesar-chen mentioned this pull request Jan 31, 2020

[Uri] Fix IPv6 address format dotnet/runtime#25782

Open

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Revert "Fix Uri.Host for IPv6 Link-local address (dotnet/corefx#29829)…

fa7034d

…" (dotnet/corefx#30062) This reverts commit dotnet/corefx@f7a8cf1. Commit migrated from dotnet/corefx@e5bb0b5

Fix Uri.Host for IPv6 Link-local address #29829

Fix Uri.Host for IPv6 Link-local address #29829

Conversation

MarcoRossignoli commented May 21, 2018 • edited Loading

MarcoRossignoli commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli May 22, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented May 22, 2018

MarcoRossignoli commented May 22, 2018

MarcoRossignoli commented May 22, 2018

stephentoub commented May 22, 2018

MarcoRossignoli commented May 22, 2018 • edited Loading

caesar-chen commented May 22, 2018

MarcoRossignoli commented May 23, 2018 • edited Loading

MarcoRossignoli commented May 24, 2018

MarcoRossignoli May 24, 2018 • edited Loading

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

stephentoub May 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub May 24, 2018 • edited Loading

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

MarcoRossignoli commented May 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesar-chen commented May 25, 2018

MarcoRossignoli commented May 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli commented May 25, 2018

caesar-chen commented May 25, 2018

MarcoRossignoli commented May 25, 2018

MarcoRossignoli commented May 26, 2018

caesar-chen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli May 28, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli commented May 28, 2018

rmkerr left a comment

Choose a reason for hiding this comment

MarcoRossignoli commented May 21, 2018 •

edited

Loading

MarcoRossignoli May 21, 2018 •

edited

Loading

MarcoRossignoli May 22, 2018 •

edited

Loading

MarcoRossignoli commented May 22, 2018 •

edited

Loading

MarcoRossignoli commented May 23, 2018 •

edited

Loading

MarcoRossignoli May 24, 2018 •

edited

Loading

stephentoub May 24, 2018 •

edited

Loading

stephentoub May 24, 2018 •

edited

Loading

MarcoRossignoli May 28, 2018 •

edited

Loading