Richfr/ Add windowsAddress support to WslcCreateContainer() API#40037
Richfr/ Add windowsAddress support to WslcCreateContainer() API#400371wizkid wants to merge 18 commits intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the WSLC Client SDK’s container port-mapping path to support custom host binding addresses (including IPv6) instead of always binding to 127.0.0.1, and relaxes/updates validation around the windowsAddress field.
Changes:
- Convert
windowsAddress(IPv4/IPv6) into a string binding address forWSLCPortMappingduring container creation. - Add input validation for
windowsAddress->ss_familyinWslcSetContainerSettingsPortMappings.
There was a problem hiding this comment.
Almost certainly needs a test update to prevent a negative test from failing when it passes in an address. Not sure if we can expect IPv6 everywhere, but at least passing in 127.0.0.1 (or maybe IPv6 loopback works even if no external IPv6 address is available?).
[Edit: Search suggests that IPv6 loopback is always available if device supports IPv6.]
OneBlue
left a comment
There was a problem hiding this comment.
Added a couple comments, I would also recommend:
- Updating the pull request title
- Adding test coverage for the binding address wiring (at least for localhost, since that's the only thing that the service supports today)
You'll probably have to update existing tests as well
|
|
||
| default: | ||
| // Reject unsupported or malformed address families | ||
| THROW_HR(E_INVALIDARG); |
There was a problem hiding this comment.
nit: I recommend THROW_HR_MSG() so we can log the address family we received to make debugging easier
src/windows/WslcSDK/wslcsdk.cpp
Outdated
| else | ||
| { | ||
| // If no binding address is provided, default to wildcard. | ||
| convertedPort.Family = AF_UNSPEC; |
There was a problem hiding this comment.
Won't this fail later if we leave AF_UNSPEC here ?
I would recommend either defaulting to localhost, or requiring the caller to pass in the binding address.
The later would be my preference, so that way there's no ambiguity
|
Hey @1wizkid 👋 — Following up on this PR. Currently the wslc tests are failing in CI, and there are 11 unresolved review threads with feedback from maintainers, including:
Is this change still needed? The review feedback covers some important safety and design issues that should be addressed before this can move forward. Let us know if you need any help! |
|
Ill focus on this today.
Thanks,
Richard
Sent from my T-Mobile 5G Device
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
|
Pull request was closed
7f1eca4 to
715ce52
Compare
…ate windowAddress values if present.
…s to the correct HRESULTS
…that convertedPorts must stay in same scope as containerOptions and moved the declaration to be with that var
…for Ipv4 and Ipv6 local host.
…log reports easier.
|
The bulk of the issues addressed and new tests submitted as well that cover the usage of windowsAddress IPv4 & IPv6.
Waiting for the test pass to complete and will see if anything left to do.
Thanks,
Richard
|
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed