Test-Connection: Fallback to hop IP Address on -Traceroute without -ResolveDestination #11335

vexx32 · 2019-12-13T05:43:24Z

PR Summary

When doing traceroutes, Destination of the PingStatus is set as the router name.
If no router name is available, it ends up as string.Empty.

This PR changes the logic in ProcessTraceroute() to instead assign the hopAddressString value as the Destination field for the PingStatus object created if there is no routerName available.

PR Context

Fixes #11334

PR Checklist

vexx32 · 2019-12-13T05:46:34Z

/cc @daxian-dbw @SteveL-MSFT I know it's late in the cycle, but I'd really rather not leave this bug in for v7 😅 so if we're able to tidy this loose end away for RC or GA that'd be much appreciated! 💖

iSazonov · 2019-12-13T08:07:43Z

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

+                get
+                {
+                    if (_status.Address?.ToString() == IPAddress.Any.ToString()
+                        || _status.Address?.ToString() == IPAddress.IPv6Any.ToString())
+                    {
+                        // There was no response to the ping (TimedOut).
+                        return null;
+                    }
+
+                    if (_status.Destination == string.Empty)
+                    {
+                        // There was a response, but the destination field is empty; use DisplayAddress.
+                        return _status.DisplayAddress;
+                    }
+
+                    return _status.Destination;


Looking the code I guess there is a bug in ProcessTraceroute()
if (ResolveDestination.IsPresent && routerName == string.Empty) - we skip assigning the routerName

Yeah. I left that in because I wanted to allow it to retry assigning the router name if resolve destination is present. I think I should split up that if statement, so if ResolveDestination isn't present at all it just immediately assigns the IP address.

Fixed the ProcessTraceroute() issue by checking if routerName is string.Empty when creating the objects for output and instead assigning the hopAddressString as the Destination field instead of leaving it blank.

vexx32 · 2019-12-13T16:59:04Z

Huh, looks like that commit isn't showing up yet. Checked https://www.githubstatus.com/ and it looks liek they're having some issues. I'll check back tomorrow and see if it shows, I might need to push an empty commit to get things to refresh here.

EDIT: It showed up, so we should be good! 🎉

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

iSazonov · 2019-12-14T09:18:26Z

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

-#if !UNIX
-                        if (ResolveDestination.IsPresent && routerName == string.Empty)
+                        if (routerName == string.Empty
+                            || ResolveDestination.IsPresent && routerName == hopAddressString)


Why do we need InitProcessPing() in the for cycle if we ping the same address?

Mainly for -ResolveDestination purposes, to retrieve the DNS name for the hop point. Note that it is skipped if there is already a name retrieved for that hop point.

We call InitProcessPing() for the same hop in the cycle. I suggest to move the call out of the cycle.

Moved the InitProcessPing() call to just before the loop. 👍

iSazonov · 2019-12-14T09:25:39Z

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

+                    if (_status.Address == IPAddress.Any
+                        || _status.Address == IPAddress.IPv6Any)
+                    {
+                        // There was no response to the ping (TimedOut).
+                        return null;
+                    }
+
+                    // We have a usable IP address from this ping reply.
+                    return _status.Destination;


We initialize TraceStatus with resolvedTargetName coming from InitProcessPing(). So maybe we should do the check and assign to null in InitProcessPing() ?

// There was no response to the ping (TimedOut).

I see we use targetAddress to initialize TraceStatus but the value is not related to ping status and timeout.

Not 100% clear on what you're suggesting here. Just move this check to the constructor and set the value there?

Yes, TraceStatus isn't directly tied to PingStatus, but it does effectively "wrap" the PingStatus and surfaces some of the properties from the PingStatus while adding some extra properties to make the traceroute progress clear.

Moved this check to the constructor so we only need to check it once. 👍

PoshChan · 2019-12-14T17:05:20Z

@vexx32, this is the reminder you requested 24 hours. ago

iSazonov · 2019-12-15T07:57:44Z

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

@@ -383,25 +381,14 @@ private void ProcessTraceroute(string targetNameOrAddress)
 #endif
                var hopAddressString = discoveryReply.Address.ToString();

+                InitProcessPing(hopAddressString, out string routerName, out _);


We should still put this in try-catch.

You mean have an if/else? The method should be catching all errors here.

I'll add an if statement so we properly set some sort of router name based on hopAddressString if the name can't be resolved, etc.

No, the method throws if router name can not be resolved

Suggested change

InitProcessPing(hopAddressString, out string routerName, out _);

try

{

InitProcessPing(hopAddressString, out string routerName, out _);

}

catch

{

}

The method writes an error, but it does not throw.

https://github.com/PowerShell/PowerShell/blob/f5ab069257c1e41ffc294ebb3676ae5c8830eb96/src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs#L637-L730

I tested with artifact and get throw on first hop where router hasn't name. I guess it was in GetHostEntry().

I still see the issue in latest artifact

PS C:\Users\1\Downloads\publish> Get-Error Exception : Type : System.Net.Sockets.SocketException Message : No such host is known. SocketErrorCode : HostNotFound ErrorCode : 11001 NativeErrorCode : 11001 TargetSite : Name : InternalGetHostByAddress DeclaringType : System.Net.Dns MemberType : Method Module : System.Net.NameResolution.dll StackTrace : at System.Net.Dns.InternalGetHostByAddress(IPAddress address) at System.Net.Dns.GetHostEntry(String hostNameOrAddress) at Microsoft.PowerShell.Commands.TestConnectionCommand.TryResolveNameOrAddress(String targetNameOrAddress, String& r esolvedTargetName, IPAddress& targetAddress) at Microsoft.PowerShell.Commands.TestConnectionCommand.ProcessTraceroute(String targetNameOrAddress) at Microsoft.PowerShell.Commands.TestConnectionCommand.ProcessRecord() at System.Management.Automation.CommandProcessor.ProcessRecord() Source : System.Net.NameResolution HResult : -2147467259 CategoryInfo : NotSpecified: (:) [Test-Connection], SocketException FullyQualifiedErrorId : System.Net.Sockets.SocketException,Microsoft.PowerShell.Commands.TestConnectionCommand InvocationInfo : MyCommand : Test-Connection ScriptLineNumber : 1 OffsetInLine : 1 HistoryId : 4 Line : Test-Connection -Traceroute 1.1.1.1 -ResolveDestination PositionMessage : At line:1 char:1 + Test-Connection -Traceroute 1.1.1.1 -ResolveDestination + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ InvocationName : Test-Connection CommandOrigin : Internal ScriptStackTrace : at <ScriptBlock>, <No file>: line 1

Huh! GetHostEntry itself throws? Thanks for following up on that. Yeah, I can sort that out. 🙂

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

- When doing traceroutes, Destination is set as the router name. If no router name is available, it ends up as string.Empty. - Change Hostname property to fallback to DisplayAddress if needed.

- Previously, the Destination field of the PingStatus could be empty - Change ensures that it will always be populated with the IP address if no hostname is available. - The getter for TraceStatus.Hostname was adjusted to make better use of the underlying properties as well.

- Before this change, if ResolveDestination was not set, we did not use the method to get the hop address string. - After this change, the routerName and thus Destination field will always be set.

- Call InitProcessPing() only once per hop. - Set Hostname in TraceStatus constructor.

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

SteveL-MSFT

Would it be correct that with this change, test-connection 127.0.0.1 -traceroute should not output any asterisks? If so, we can just test that case.

vexx32 · 2019-12-17T22:16:54Z

Yeah, that should work. 👍

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

SteveL-MSFT

LGTM

This resolves an issue where intermediate hops may not have DNS entries. Retaining this behaviour means we acknowledge not all nodes in the trace will necessarily be reachable. This is quite normal; many nodes may block direct ICMP requests. By catching any errors during hostname resolution, we get a more useful experience for users.

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs

daxian-dbw · 2019-12-18T17:45:49Z

@vexx32 Can you please look into the build failure?

  System.Management.Automation -> /home/vsts/work/1/s/src/System.Management.Automation/bin/Release/netcoreapp3.1/System.Management.Automation.dll
  Microsoft.PowerShell.ConsoleHost -> /home/vsts/work/1/s/src/Microsoft.PowerShell.ConsoleHost/bin/Release/netcoreapp3.1/Microsoft.PowerShell.ConsoleHost.dll
  Microsoft.PowerShell.MarkdownRender -> /home/vsts/work/1/s/src/Microsoft.PowerShell.MarkdownRender/bin/Release/netcoreapp3.1/Microsoft.PowerShell.MarkdownRender.dll
  Microsoft.PowerShell.Security -> /home/vsts/work/1/s/src/Microsoft.PowerShell.Security/bin/Release/netcoreapp3.1/Microsoft.PowerShell.Security.dll
  Microsoft.PowerShell.Commands.Utility -> /home/vsts/work/1/s/src/Microsoft.PowerShell.Commands.Utility/bin/Release/netcoreapp3.1/Microsoft.PowerShell.Commands.Utility.dll
/home/vsts/work/1/s/src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs(384,37): error CS8600: Converting null literal or possible null value to non-nullable type. [/home/vsts/work/1/s/src/Microsoft.PowerShell.Commands.Management/Microsoft.PowerShell.Commands.Management.csproj]
Execution of { dotnet $Arguments } by build.psm1: line 440 failed with exit code 1

vexx32 · 2019-12-18T18:04:57Z

@daxian-dbw Ah yep, that'll do it. Still getting used to working in files with nullable reference types enabled.

iSazonov · 2019-12-18T18:09:14Z

I like nullable reference types more and more.

vexx32 · 2019-12-18T18:41:08Z

They really help keep things locked down tight, it's lovely. I just have to remember to be a little careful when working with them. 😁

…`-ResolveDestination` (#11335)

ghost · 2020-01-16T23:37:31Z

🎉v7.0.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

vexx32 requested review from adityapatwardhan and daxian-dbw as code owners December 13, 2019 05:43

ghost assigned daxian-dbw Dec 13, 2019

vexx32 added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Dec 13, 2019

iSazonov reviewed Dec 13, 2019

View reviewed changes

PoshChan reviewed Dec 13, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

iSazonov reviewed Dec 13, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

iSazonov reviewed Dec 13, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

iSazonov reviewed Dec 14, 2019

View reviewed changes

iSazonov reviewed Dec 15, 2019

View reviewed changes

daxian-dbw added this to the 7.0-Consider milestone Dec 16, 2019

adityapatwardhan modified the milestones: 7.0-Consider, GA-consider Dec 16, 2019

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

vexx32 added 7 commits December 17, 2019 11:40

🐛 Fallback to DisplayAddress if no Destination

3af7126

- When doing traceroutes, Destination is set as the router name. If no router name is available, it ends up as string.Empty. - Change Hostname property to fallback to DisplayAddress if needed.

🐛 Refactor to use InitProcessPing

4399a9c

- Before this change, if ResolveDestination was not set, we did not use the method to get the hop address string. - After this change, the routerName and thus Destination field will always be set.

🔥 Remove unnecessary ToString() calls

0304d01

♻️ Refactor traceroute and tracestatus

143b785

- Call InitProcessPing() only once per hop. - Set Hostname in TraceStatus constructor.

♻️ Add if/else for per-hop InitProcessPing

55e9f71

♻️ Fix null issue in syntax

72e9e8f

vexx32 force-pushed the TestConnection/FixTraceHostnames branch from 754f062 to 72e9e8f Compare December 17, 2019 17:40

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

vexx32 changed the title ~~Test-Connection: Fallback to DisplayAddress on -Traceroute without -ResolveDestination~~ Test-Connection: Fallback to hop IP Address on -Traceroute without -ResolveDestination Dec 17, 2019

SteveL-MSFT reviewed Dec 17, 2019

View reviewed changes

✅ Verify Destination works

8f305bb

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

PoshChan reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Show resolved Hide resolved

vexx32 added 2 commits December 17, 2019 17:04

✅ Fix test

c14da69

♻️ Rename InitProcessPing()

b1db89a

SteveL-MSFT reviewed Dec 17, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

♻️ CanResolve..()->TryResolve..()

6fa2247

SteveL-MSFT approved these changes Dec 18, 2019

View reviewed changes

iSazonov approved these changes Dec 18, 2019

View reviewed changes

src/Microsoft.PowerShell.Commands.Management/commands/management/TestConnectionCommand.cs Outdated Show resolved Hide resolved

🐛 Define routerName in proper context

4fc9fb6

🐛 Fix non-nullable oops

8e58e9b

daxian-dbw merged commit 7b33cfe into PowerShell:master Dec 19, 2019

vexx32 deleted the TestConnection/FixTraceHostnames branch December 19, 2019 22:53

adityapatwardhan modified the milestones: GA-consider, GA-approved Jan 7, 2020

daxian-dbw pushed a commit that referenced this pull request Jan 10, 2020

Test-Connection: Fallback to hop IP Address on -Traceroute without …

2fe9b2f

…`-ResolveDestination` (#11335)

daxian-dbw modified the milestones: GA-approved, 7.0.0-rc.2 Jan 11, 2020

ghost mentioned this pull request Jan 16, 2020

Test-Connection -Traceroute doesn't show hostnames at all #11334

Closed

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Update list of PowerShell team members in release tools #18266

Merged

22 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test-Connection: Fallback to hop IP Address on -Traceroute without -ResolveDestination #11335

Test-Connection: Fallback to hop IP Address on -Traceroute without -ResolveDestination #11335

vexx32 commented Dec 13, 2019 •

edited

Loading

vexx32 commented Dec 13, 2019

iSazonov Dec 13, 2019

vexx32 Dec 13, 2019

vexx32 Dec 13, 2019

vexx32 commented Dec 13, 2019 •

edited

Loading

iSazonov Dec 14, 2019

vexx32 Dec 14, 2019

iSazonov Dec 14, 2019 •

edited

Loading

vexx32 Dec 15, 2019

iSazonov Dec 14, 2019

vexx32 Dec 15, 2019

vexx32 Dec 15, 2019

PoshChan commented Dec 14, 2019

iSazonov Dec 15, 2019

vexx32 Dec 16, 2019

iSazonov Dec 17, 2019

vexx32 Dec 17, 2019

iSazonov Dec 17, 2019

iSazonov Dec 18, 2019 •

edited

Loading

vexx32 Dec 18, 2019

SteveL-MSFT left a comment

vexx32 commented Dec 17, 2019

SteveL-MSFT left a comment

daxian-dbw commented Dec 18, 2019 •

edited

Loading

vexx32 commented Dec 18, 2019

iSazonov commented Dec 18, 2019

vexx32 commented Dec 18, 2019

ghost commented Jan 16, 2020

Test-Connection: Fallback to hop IP Address on -Traceroute without -ResolveDestination #11335

Test-Connection: Fallback to hop IP Address on -Traceroute without -ResolveDestination #11335

Conversation

vexx32 commented Dec 13, 2019 • edited Loading

PR Summary

PR Context

PR Checklist

vexx32 commented Dec 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vexx32 commented Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov Dec 14, 2019 • 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

PoshChan commented Dec 14, 2019

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

iSazonov Dec 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SteveL-MSFT left a comment

Choose a reason for hiding this comment

vexx32 commented Dec 17, 2019

SteveL-MSFT left a comment

Choose a reason for hiding this comment

daxian-dbw commented Dec 18, 2019 • edited Loading

vexx32 commented Dec 18, 2019

iSazonov commented Dec 18, 2019

vexx32 commented Dec 18, 2019

ghost commented Jan 16, 2020

vexx32 commented Dec 13, 2019 •

edited

Loading

vexx32 commented Dec 13, 2019 •

edited

Loading

iSazonov Dec 14, 2019 •

edited

Loading

iSazonov Dec 18, 2019 •

edited

Loading

daxian-dbw commented Dec 18, 2019 •

edited

Loading