WebCmdlet unix-sockets by CarloToso · Pull Request #19343 · PowerShell/PowerShell

CarloToso · 2023-03-16T00:01:10Z

PR Summary

Replaces #18995 after the changes in #19249

Replaces HttpClientHanlder with SocketsHttpHandler
Adds support for UnixSockets
Now passes all the tests

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"

TODO:

~~Add specific tests (Help Wanted)~~ DONE
~~Add documentation~~ DONE

PR Context

fix #12060
fix #8314

PR Checklist

CarloToso · 2023-03-16T10:49:31Z

Unrelated test error

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

SteveL-MSFT

LGTM

ghost · 2023-04-04T02:00:52Z

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

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx

xtqqczze · 2023-04-12T01:30:43Z

I wonder whether we should handle Socket.OSSupportsUnixDomainSockets is false during argument validation, or later in CreateHttpClient. In any case, would it be better for the error to have ErrorCategory.NotImplemented?

iSazonov · 2023-04-12T04:55:44Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+            get => _unixSocket; 
+            set
+            {
+                if (Socket.OSSupportsUnixDomainSockets is true)


Is it possible to create UnixSocket if Socket.OSSupportsUnixDomainSockets is false? I guess no. So it makes no sense to add the check.

@iSazonov unfortunately I can't check at the moment, could you test what happens if Socket.OSSupportsUnixDomainSockets is false?

UnixDomainSocketEndPoint throws https://source.dot.net/#System.Net.Sockets/System/Net/Sockets/UnixDomainSocketEndPoint.cs,59

Actually System.Net.Sockets.Socket..ctor(AddressFamily, SocketType, ProtocolType) will throw a SocketException with the message "An address incompatible with the requested protocol was used":

https://github.com/dotnet/runtime/blob/2c8cb129ed084e374f06eaa29f21e9ab0b9e8066/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs#L81-L100

The implementation an Windows will attempt to create create a socket by calling the underlying Winsock API:

https://github.com/dotnet/runtime/blob/2c8cb129ed084e374f06eaa29f21e9ab0b9e8066/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Windows.cs#L39-L57

Is it possible to create UnixSocket if Socket.OSSupportsUnixDomainSockets is false? I guess no. So it makes no sense to add the check.

~~We probably want to perform the check so that we can create the appropriate ErrorRecord with a more informative message, i.e.:~~

~~if (!Socket.OSSupportsUnixDomainSockets) ThrowTerminatingError(GetUnixDomainSocketNotSupportedError()) ```</strike>~~

~~That being said, the error handling of System.Net.Sockets.SocketPal.CreateSocket could be improved.~~

…trings.resx Co-authored-by: xtqqczze <[email protected]>

test/tools/Modules/UnixSocket/UnixSocket.psm1

Co-authored-by: xtqqczze <[email protected]>

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

ghost · 2023-06-05T14:01:01Z

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

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

CarloToso · 2023-07-10T19:53:37Z

@SteveL-MSFT
It was handled in the following commits following @xtqqczze 's suggestions:
d9ec5c4; bd5c4d6; f7ec7e8; 2b9a2b9
and removed as asked by @iSazonov (#19343 (comment))
e7090f3

xtqqczze · 2023-07-11T13:20:29Z

@SteveL-MSFT It was handled in the following commits following @xtqqczze 's suggestions: d9ec5c4; bd5c4d6; f7ec7e8; 2b9a2b9 and removed as asked by @iSazonov (#19343 (comment)) e7090f3

I agree with @iSazonov, error handling is not required, see #19343 (comment).

microsoft-github-policy-service · 2023-07-18T23:11:12Z

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

microsoft-github-policy-service · 2023-07-18T23:11:13Z

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

LGTM

daxian-dbw · 2023-07-24T16:56:05Z

Context
#12060 and #8314

Does this PR fix those 2 issues? Can they be closed after merging this PR?

CarloToso · 2023-07-24T17:13:57Z

Context
#12060 and #8314

Does this PR fix those 2 issues? Can they be closed after merging this PR?

Yes

daxian-dbw · 2023-07-24T17:17:09Z

@CarloToso The markdown spelling check failed. Please fix the markdown spelling issues.

pull-request-quantifier-deprecated · 2023-07-24T22:05:54Z

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

Quantification details

Label      : Medium
Size       : +164 -12
Percentile : 55.2%

Total files changed: 10

Change summary by file extension:
.spelling : +1 -0
.psm1 : +77 -1
.cs : +34 -11
.ps1 : +13 -0
.md : +12 -0
.psd1 : +16 -0
.csproj : +11 -0

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 added 2 commits March 16, 2023 00:55

Unix sockets

e2fbf1e

rework _custommethod

550707e

CarloToso requested a review from PaulHigin as a code owner March 16, 2023 00:01

pull-request-quantifier-deprecated bot added the Extra Small label Mar 16, 2023

ghost assigned adityapatwardhan Mar 16, 2023

CarloToso mentioned this pull request Mar 16, 2023

WIP: Add UnixSocket to WebCmdlets #18995

Closed

22 tasks

CarloToso added 2 commits March 16, 2023 10:47

use SetClassVar

ec2fcf1

Pass UnixSocket to WebSession even if it's null

84968e7

CarloToso added 3 commits March 16, 2023 16:08

Merge branch 'PowerShell:master' into WebCmdlets-UnixSockets2

95d2b33

UnixDomainSocketEndPoint nullable

30132a8

remove useless line

43f2772

SteveL-MSFT reviewed Mar 20, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Show resolved Hide resolved

SteveL-MSFT approved these changes Mar 27, 2023

View reviewed changes

ghost added the Review - Needed The PR is being reviewed label Apr 4, 2023

xtqqczze reviewed Apr 11, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Show resolved Hide resolved

CarloToso added 2 commits April 12, 2023 02:08

Add Socket.OSSupportsUnixDomainSockets validation

d9ec5c4

move error to ValidateParameters()

bd5c4d6

xtqqczze reviewed Apr 12, 2023

View reviewed changes

src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletStrings.resx Outdated Show resolved Hide resolved

iSazonov reviewed Apr 12, 2023

View reviewed changes

CarloToso and others added 5 commits April 12, 2023 09:44

Update src/Microsoft.PowerShell.Commands.Utility/resources/WebCmdletS…

f7ec7e8

…trings.resx Co-authored-by: xtqqczze <[email protected]>

update error

2b9a2b9

remove error

e7090f3

fix UseDefaultCredentials

87ed139

Add Tests

3712927

CarloToso requested review from TravisEz13, adityapatwardhan and daxian-dbw as code owners April 28, 2023 18:41

Add $using:

22ae762

xtqqczze reviewed May 27, 2023

View reviewed changes

test/tools/Modules/UnixSocket/UnixSocket.psm1 Outdated Show resolved Hide resolved

Update test/tools/Modules/UnixSocket/UnixSocket.psm1

aa5e0ed

Co-authored-by: xtqqczze <[email protected]>

xtqqczze reviewed May 27, 2023

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated Show resolved Hide resolved

CarloToso added 2 commits May 27, 2023 15:42

remove After-All

8eda977

change extension to sock

62b1b88

ghost added the Review - Needed The PR is being reviewed label Jun 5, 2023

SteveL-MSFT requested changes Jul 10, 2023

View reviewed changes

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs Show resolved Hide resolved

ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed labels Jul 10, 2023

Merge branch 'master' into WebCmdlets-UnixSockets2

3627347

ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 10, 2023

microsoft-github-policy-service bot added the Review - Needed The PR is being reviewed label Jul 18, 2023

SteveL-MSFT approved these changes Jul 24, 2023

View reviewed changes

microsoft-github-policy-service bot removed the Review - Needed The PR is being reviewed label Jul 24, 2023

CarloToso added 2 commits July 24, 2023 23:34

Merge branch 'PowerShell:master' into WebCmdlets-UnixSockets2

48df0f8

fix MD025 - Multiple top-level headings in the same document

352b050

daxian-dbw merged commit e8ce6a5 into PowerShell:master Jul 24, 2023

daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 24, 2023

CarloToso deleted the WebCmdlets-UnixSockets2 branch July 25, 2023 10:37

Conversation

CarloToso commented Mar 16, 2023 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

CarloToso commented Mar 16, 2023

Uh oh!

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 4, 2023

Uh oh!

Uh oh!

Uh oh!

xtqqczze commented Apr 12, 2023

Uh oh!

iSazonov Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

CarloToso Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

iSazonov Apr 12, 2023

Choose a reason for hiding this comment

Uh oh!

xtqqczze Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xtqqczze Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ghost commented Jun 5, 2023

Uh oh!

Uh oh!

CarloToso commented Jul 10, 2023

Uh oh!

xtqqczze commented Jul 11, 2023

Uh oh!

microsoft-github-policy-service bot commented Jul 18, 2023

Uh oh!

microsoft-github-policy-service bot commented Jul 18, 2023

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented Jul 24, 2023

Uh oh!

CarloToso commented Jul 24, 2023

Uh oh!

daxian-dbw commented Jul 24, 2023

Uh oh!

pull-request-quantifier-deprecated bot commented Jul 24, 2023

What can I do to optimize my changes

How to interpret the change counts in git diff output

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

CarloToso commented Mar 16, 2023 •

edited by daxian-dbw

Loading

xtqqczze Apr 12, 2023 •

edited

Loading

xtqqczze Apr 12, 2023 •

edited

Loading

xtqqczze Apr 12, 2023 •

edited

Loading

xtqqczze Apr 12, 2023 •

edited

Loading