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

Windows: Fix address_imp_test #11334

Closed

Conversation

davinci26
Copy link
Member

@davinci26 davinci26 commented May 27, 2020

Commit Message:
Enable Abstract sockets for Windows and fix test permissions

Additional Description:

With this change we aim to add full support for the address_imp_test on Windows. To achieve this we made the following changes:

  1. In BasicPermission we set the mode to be 666. This allows us to verify in linux that we set the permissions of the socket correctly. This is the equivalent to _S_IREAD | _S_IWRITE in Windows. In case that we need more granular permission model, we would need to use ACL in Windows.
  2. Since Windows AF_UNIX sockets support abstract sockets source I enabled all the abstract tests in Windows.

Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11334 was opened by davinci26.

see: more, trace.

@davinci26
Copy link
Member Author

cc @sunjayBhatia @wrowe @nigriMSFT

@davinci26
Copy link
Member Author

davinci26 commented May 27, 2020

I will squash the commits when the PR is ready to get merged if this alright with everyone

@nigriMSFT
Copy link
Contributor

Is this tested? From what I have read, abstract names don't work with AF_UNIX on Windows yet. Sunil confirmed that the documentation is incorrect in microsoft/WSL#4240

@davinci26
Copy link
Member Author

@nigriMSFT I run the envoy tests to validate it. I will look into the issue as well

@wrowe
Copy link
Contributor

wrowe commented May 27, 2020

As we understood the public facing blogs and notes, abstract names aren't supported on Windows, nor apparently on apple (bsd-derived socket stack). If this is true, we should be able to normalize throughout against defined(__linux__) as the variant to supporting abstract sockets.

@davinci26
Copy link
Member Author

I also confirmed it internally that abstract names aren't supported on Windows for security reasons.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants