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

Test-Connection needs to be revised due to an intentional breaking change in .NET 7 #17018

Closed
5 tasks done
daxian-dbw opened this issue Mar 17, 2022 · 6 comments · Fixed by #20369
Closed
5 tasks done
Assignees
Labels
In-PR Indicates that a PR is out for the issue Issue-Bug Issue has been identified as a bug in the product WG-Cmdlets general cmdlet issues

Comments

@daxian-dbw
Copy link
Member

daxian-dbw commented Mar 17, 2022

Prerequisites

Steps to reproduce

Here is the dotnet issue: dotnet/runtime#66746
Here is the breaking change note: dotnet/docs#28720

Basically, System.Net.NetworkInformation.Ping doesn't support specifying payload on Linux:

  • it throws PlatformNotSupported exception when custom Ping payload is specified, when the process is not running as root.
  • even if running as root (sudo), the custom payload is simply ignored because on Linux it's implemented by interfacing with the ping native utility, which doesn't support specifying custom payload.

So, whatever functionality in Test-Connection that depends on specifying custom payload is broken and should be either fixed or removed.

Attention: Workaround was added to test code in Test-Connection.Tests.ps1 by #16930. Please fix the tests when resolving this bug, or removing the related tests if the affected functionalities will be removed.

Expected behavior

N/A

Actual behavior

N/A

Error details

N/A

Environment data

PowerShell 7.3-preview.3

Visuals

No response

@daxian-dbw daxian-dbw added Needs-Triage The issue is new and needs to be triaged by a work group. Issue-Bug Issue has been identified as a bug in the product WG-Cmdlets general cmdlet issues and removed Needs-Triage The issue is new and needs to be triaged by a work group. labels Mar 17, 2022
@daxian-dbw
Copy link
Member Author

@iSazonov You may want to take a look at this issue.

@daxian-dbw daxian-dbw changed the title Test-Connection needs to be revised due to an internal breaking change in .NET 7 Test-Connection needs to be revised due to an intentional breaking change in .NET 7 Mar 21, 2022
@vexx32
Copy link
Collaborator

vexx32 commented Nov 9, 2022

My read of the linked issue and related issues/PRs seems to indicate that the content of a custom payload is ignored, but the size of the payload is still passed to the ping native util if running as root or with proper capabilities (permissions?) set on the calling process.

So I guess the question here is -- is this enough of a feature that we think it worth keeping for that (kinda narrow imo) use case, and wrap the error that comes up to tell the user to run pwsh as root if they want to use the relevant parameter? Or do we just compile this on Unix systems without that parameter entirely / ignore it if it's set?

@vexx32 vexx32 added the Needs-Triage The issue is new and needs to be triaged by a work group. label Nov 9, 2022
@JoeSalmeri
Copy link

@vexx32

So I guess the question here is -- is this enough of a feature that we think it worth keeping for that (kinda narrow imo) use
case, and wrap the error that comes up to tell the user to run pwsh as root if they want to use the relevant parameter? Or do > we just compile this on Unix systems without that parameter entirely / ignore it if it's set?

Is anything going to be done to address the issue with Test-Connection now requiring root on Linux?

Standard ping command works fine on Linux as a regular user.

At a minimum add an option to Test-Connection that allows it to work for regular users like standard ping command does.

So often I see MS's answer that they won't fix something because it might/could break existing customer code that depends on that behavior and yet this change which actual broke all my code uses of Test-Connection is allowed to stand.

Maybe MS needs to change their answer to "Won't fix something because it might/could break existing customer code that depends on that behavior except for all the exceptions that we make".

Guess I write my own Test-Connection around the built in ping command to get a workable solution as it doesn't sound like this will be addressed.

@mcx808
Copy link

mcx808 commented Jun 9, 2023

+1 - I came to open a new issue on this but found this one. Issue present in 7.3.4
Test-Connection should work for ICMP without sudo/root regardless of what the underlying .Net function is doing. Ping doesn't require sudo, neither does testing a TCP port.
Issue is specific to Linux as macOS also works fine so it's not purely a *nix issue.

@SteveL-MSFT
Copy link
Member

Looking at the code and the .NET change, it appears that the problem is that we just need to remove

for (int i = 0; i < bufferSize; i++)
{
sendBuffer[i] = (byte)((int)'a' + i % 23);
}
so that the default buffer of 32 works. We also need to document that on Linux, any buffer size that is not the default requires sudo.

@SteveL-MSFT SteveL-MSFT removed the Needs-Triage The issue is new and needs to be triaged by a work group. label Jul 19, 2023
@JoeSalmeri
Copy link

@SteveL-MSFT

Thanks Steve that would be an appreciated solution.

Is there an ETA as to what pwsh release that change will target ?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In-PR Indicates that a PR is out for the issue Issue-Bug Issue has been identified as a bug in the product WG-Cmdlets general cmdlet issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants