Skip to content

WIP: Add UnixSocket to WebCmdlets #18995

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.

Sign up for GitHub

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

Jump to bottom
Closed
CarloToso wants to merge 11 commits into from
Closed

WIP: Add UnixSocket to WebCmdlets #18995

CarloToso wants to merge 11 commits into from

Conversation

@CarloToso
Copy link
Contributor

@CarloToso CarloToso commented Jan 20, 2023 •
edited
Loading

PR Summary

Untested and in an early stage, it probably won't work yet.
This PR aims to add -UnixSocket support to Invoke-WebRequest and Invoke-RestMehtod

  • Switched from System.Net.HttpClientHandler to System.Net.Http.SocketsHttpHandler to expose ConnectCallback
  • Replaced handler options (couldn't find a replacement for handler.ClientCertificateOptions = ClientCertificateOption.Manual;, it might no longer be necessary)
  • Added -UnixSocket parameter

Syntax:
curl --unix-socket /var/run/docker.sock http://v1.40/images/json
Invoke-WebRequest -Uri "http://v1.40/images/json/" -UnixSocket "/var/run/docker.sock"

The tests highlight some problems with the Certificates -> when using both -Certificate and -SkipCertificateCheck on Linux I think we'll have to update the 2 failing tests

PR Context

#12060 and #8314

PR Checklist

@CarloToso CarloToso requested a review from PaulHigin as a code owner January 20, 2023 17:09
@ghost ghost assigned TravisEz13 Jan 20, 2023
@CarloToso CarloToso changed the title [WIP] Add UnixSocket to WebCmdlets Add UnixSocket to WebCmdlets Jan 21, 2023
@iSazonov iSazonov requested a review from JamesWTruher January 23, 2023 07:04
@CarloToso CarloToso mentioned this pull request Jan 23, 2023
Closed
@ghost ghost added the Review - Needed The PR is being reviewed label Jan 30, 2023
@ghost
Copy link

ghost commented Jan 30, 2023

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@SteveL-MSFT SteveL-MSFT changed the title Add UnixSocket to WebCmdlets WIP: Add UnixSocket to WebCmdlets Jan 30, 2023
@CarloToso
Copy link
Contributor Author

CarloToso commented Feb 4, 2023

I finally had time to test this PR. It works correctly in WSL1 but I'm not able to connect to WSL1 Unix-Sockets server from Windows (this is not a pwsh problem, I have the same problem with curl).

Cattura

@CarloToso
Copy link
Contributor Author

CarloToso commented Feb 4, 2023 •
edited
Loading

After the latest change the tests work on Windows and MacOS but still fail on Linux. The error originates from the -Certificate parameter. @rjmholt you made the SelfSignedCertificate module, maybe you can help me solve this problem.

@olljanat
Copy link

olljanat commented Feb 22, 2023

Looks to be working with Invoke-RestMethod also like I expected. This would be very welcome feature to PowerShell:
image

I don't know what is practice here but I would prefer that you squash those commits to one and rebase this against of master.
Then your probably should add some test case to /test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

Also does the -UnixSocket switch now appear also to Windows version? Most probably it should not or there should be at least proper error message shown for user on that case.

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 22, 2023 •
edited by unfurl-links bot
Loading

Also does the -UnixSocket switch now appear also to Windows version? Most probably it should not or there should be at least proper error message shown for user on that case.

Unix sockets are supported on Windows https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/

Windows Command Line
Introduction:  Beginning in Insider Build 17063, you’ll be able to use the unix socket (AF_UNIX) address family on Windows to communicate between Win32 processes. Unix sockets allow inter-process communication (IPC) between processes on the same machine.  Overview:  Support for the unix socket has existed both in BSD and Linux for the longest time,

@ghost ghost removed the Review - Needed The PR is being reviewed label Feb 22, 2023
@pull-request-quantifier-deprecated
Copy link

pull-request-quantifier-deprecated bot commented Feb 22, 2023

This PR has 21 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details 

Label      : Extra Small
Size       : +15 -6
Percentile : 8.4%

Total files changed: 1

Change summary by file extension:
.cs : +15 -6

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detected.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@CarloToso
Copy link
Contributor Author

CarloToso commented Feb 22, 2023 •
edited
Loading

@olljanat I will add some tests after we figure out the failures on linux tests.
Could you test if this feature works correctly in Windows?

@olljanat
Copy link

olljanat commented Feb 24, 2023

@iSazonov are you aware of some real world application which uses unix sockets and is available for Windows?

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 24, 2023

@iSazonov are you aware of some real world application which uses unix sockets and is available for Windows?

I don't know. I guess main motivation was to open easy way to port Unix applications to Windows and develop new cross-platform applications.

@olljanat
Copy link

olljanat commented Feb 24, 2023

@iSazonov sure I get that but @CarloToso asked above testing in Windows so we need find (or build) some app to be able to do so.

@CarloToso
Copy link
Contributor Author

CarloToso commented Feb 24, 2023 •
edited by unfurl-links bot
Loading

@iSazonov @olljanat Maybe this could be a good starting point https://stackoverflow.com/questions/64713218/is-there-an-af-unix-support-on-windows-for-windows-wsl-interop-on-net-core

Stack Overflow
AF_UNIX sockets are supported on Windows starting in Windows Insider build 17093 (Windows 10 version 1803). See Windows/WSL Interop with AF_UNIX blog post on the Windows Command Line blog. There is...

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 24, 2023

We don't need to have a real application. We can create simple helper app.

@ghost ghost added the Review - Needed The PR is being reviewed label Mar 4, 2023
@ghost
Copy link

ghost commented Mar 4, 2023

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

@CarloToso CarloToso mentioned this pull request Mar 16, 2023
Merged
22 tasks
@CarloToso
Copy link
Contributor Author

CarloToso commented Mar 16, 2023

Replaced by #19343

@CarloToso CarloToso closed this Mar 16, 2023
@CarloToso CarloToso deleted the WebCmdlets-UnixSockets branch March 16, 2023 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@PaulHigin PaulHigin Awaiting requested review from PaulHigin

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

Assignees

@TravisEz13 TravisEz13

Labels

Extra Small Review - Needed The PR is being reviewed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@CarloToso @olljanat @iSazonov @TravisEz13