feat(api): Add npipe support#2018
Conversation
|
Great, thanks @olljanat |
|
Woot 🎉 |
|
I published docker image for testing purposes which contains these changes . You can test it with command: Implementation is now done on way that when you add local Docker env you just need enable "Windows containers" switch on there: NOTE! Currently that image it is build from my debug branch which also contains DialPipe workaround olljanat@0c564d7 and where I have rollbacked 61c285b as it looks to be broken (at least l always got "Unable to locate template file on disk" error with it). Test results
If I try without mapping npipe I got error like this: so it looks that npipe mapping works even on server version but I misses some permissions. @StefanScherer any ideas what would be missing? Other known issues currently:
|
|
Great job @olljanat 👍
An communication error should be raised as it is now when adding a local Unix environment when the socket file is not available. |
|
@olljanat Awesome! USER ContainerAdministratorI know Microsoft wants to deprecated this, but sometimes it's the only option. |
|
@StefanScherer yes "USER ContainerAdministrator" did the trick. Thanks. There is also now new version of Docker image ollijanatuinen/portainer:windows1803-amd64-npipe-2 Needs more testing but looks quite good. |
|
@olljanat Ha, thanks good to know. I'll try your image soon. Do you encounter problems on 1803 that still requires the go-winio fixes? |
|
@seal-ss your DialPipe workaround is currently included to image just in case. Only bigger issue which I can see now is that DockerHub authentication fails and that why image pull or service create features cannot be used but I'm investigating it. |
|
Docker pull and swarm features works on latest ollijanatuinen/portainer:windows1803-amd64-npipe-3 image. @deviantony can you review? Only known issue left is that if user try use npipe on Linux it will generate http panic errors to log but I'm not sure what is correct way to fix them? |
|
@olljanat I'll try to review this weekend and give you indications about the npipe on Linux issue. |
|
@deviantony ping |
|
Sorry @olljanat I'm a bit busy, I'll do a review tomorrow. |
|
No worries. I also found and fixed one new bug and did plenty of more testing. There is now docker image ollijanatuinen/portainer:windows1803-amd64-npipe-4 build directly from this branch without DialPipe workaround and same version Windows binaries can be found from: https://github.com/olljanat/portainer/files/2177891/portainer-with-npipe-support-pre4.zip @seal-ss I did quite lot of testing with this one I was not able to repeat that old issue easily. I did see that once on Win 10 but after restart it disappeared and I have not ever seen that with Windows Server, version 1803. So IMO we can merge this even before DialPipe fix merged. |
|
@olljanat That looks awesome! I've started your image in a Windows Server 1803 swarm (but currently just a docker run -d) Another approach may be to retry the request for the several dashboard API calls to make it more stable. |
|
True. It happens only time to time and I only see that on dashboard but it is still there. Now there is also test version ollijanatuinen/portainer:winio-pr80 which contains content on this PR + proposed fix from winio. With that one I was not able duplicate that issue anymore. |
|
@olljanat Yes, this image works stable even for the dashboard 🎉 |
d77df8a to
1fead33
Compare
deviantony
left a comment
There was a problem hiding this comment.
Heya @olljanat
I did a first review, have a look at my comments. One thing though, could you rebase the PR on the latest version of the develop branch ? It'll be easier to review it.
Thanks !
| @@ -167,19 +167,30 @@ func createDial(endpoint *portainer.Endpoint) (net.Conn, error) { | |||
| var host string | |||
There was a problem hiding this comment.
I think that we should default to url.Host here by assuming url.Scheme == tcp.
This simplifies the if/else statement below.
host := url.Host
if url.Scheme == "unix" || url.Scheme = "npipe" {
host = url.Path
}
api/http/proxy/local.go
Outdated
| package proxy | ||
|
|
||
| // unixSocketHandler represents a handler to proxy HTTP requests via a unix:// socket | ||
| // represents a handler to proxy HTTP requests via a unix:// socket and npipe:// named pipes |
There was a problem hiding this comment.
You can actually remove that comment.
api/http/proxy/manager.go
Outdated
| } | ||
| return manager.proxyFactory.newDockerHTTPProxy(endpointURL, false), nil | ||
| } | ||
| if endpointURL.Scheme == "npipe" { |
There was a problem hiding this comment.
I think that we can assume npipe:// or unix:// here depending on the platform, no need for that check.
Simply use:
return manager.proxyFactory.newLocalProxy(endpointURL.Path), nilSee my comment above regarding the local_unix.go and local_windows.go files.
api/http/proxy/factory_windows.go
Outdated
| "github.com/Microsoft/go-winio" | ||
| ) | ||
|
|
||
| func (factory *proxyFactory) newNamedPipeProxy(path string) http.Handler { |
There was a problem hiding this comment.
Should be renamed newLocalProxy.
| @@ -0,0 +1,33 @@ | |||
| // +build windows | |||
There was a problem hiding this comment.
File should be renamed local_windows.go.
api/http/proxy/factory_linux.go
Outdated
| "net/http" | ||
| ) | ||
|
|
||
| func (factory *proxyFactory) newNamedPipeProxy(path string) http.Handler { |
There was a problem hiding this comment.
This function should be renamed newLocalProxy and it's content should be the actual content of newDockerSocketProxy (relocate the function here and rename it).
| <span class="setting" ng-class="{ 'setting-active': $ctrl.state.displayTextFilter }" ng-click="$ctrl.updateDisplayTextFilter()" ng-if="$ctrl.showTextFilter"> | ||
| <i class="fa fa-search" aria-hidden="true"></i> Search | ||
| </span> | ||
| <span class="setting" ng-class="{ 'setting-active': $ctrl.columnVisibility.state.open }" uib-dropdown dropdown-append-to-body auto-close="disabled" is-open="$ctrl.columnVisibility.state.open"> |
There was a problem hiding this comment.
Could you rebase your PR on the latest develop branch? It will be easier to review if I can only focus on the changes related to this PR.
| }; | ||
|
|
||
| service.createLocalEndpoint = function() { | ||
| service.createLocalEndpoint = function(Windows) { |
There was a problem hiding this comment.
Please rename the parameter to useNamedPipe.
There was a problem hiding this comment.
Hmm. If we assume npipe:// or unix:/// based on OS then we can remove this parameter and whole Windows/Linux containers selection.
I will change it like that.
There was a problem hiding this comment.
Yes, if Portainer is running on Windows, then creating a local endpoint should default to use npipe. If running on Linux, default to use unix socket.
This is totally doable in the backend as we can create platform specific files (see my comment about local_windows.go and local_linux.go above).
But the frontend is not aware of the platform where Portainer is running. that's why #1186 introduced a Local boolean parameter in the endpoint creation payload.
There was a problem hiding this comment.
OK. I will re-check the original implementation
build/windows/nanoserver/Dockerfile
Outdated
| @@ -1,8 +1,10 @@ | |||
| FROM microsoft/nanoserver | |||
| ARG SOURCE_TAG=latest | |||
There was a problem hiding this comment.
What's the point of adding this ARG layer?
There was a problem hiding this comment.
I needed that to be able build Windows Server, version 1803 compatible version on Win 10 (docker build . --build-arg SOURCE_TAG=1803) other why it defaults to Windows Server 2016 based version.
There was a problem hiding this comment.
@seal-ss can I still use your rebase-docker-image tool to build a 1803 image? If so, I believe that this line is not required.
There was a problem hiding this comment.
Probably. I was just not familiar with that tool. I can test and remove ARG layer if that works.
There was a problem hiding this comment.
Yeah our build workflow is not public yet, I still have all the docs on my side. I can give you more details on Slack at https://portainer.io/slack/, ping me there
There was a problem hiding this comment.
Yes, the rebase-docker-image tool still should work for your COPY deployment builds.
|
|
||
| USER ContainerAdministrator | ||
|
|
||
| VOLUME C:\\data |
There was a problem hiding this comment.
Why did you choose to remove the default VOLUME statement?
There was a problem hiding this comment.
I find out on Windows Server, version 1803 where it tested these images using Docker process isolation mode that if VOLUME statement is on Dockerfile Portainer does not see C:\Data folder and wants create that but Windows things that it is there and folder creating fails. That that fails whole Portainer start process. Without that line it works correctly.
There was a problem hiding this comment.
@seal-ss are you aware of any issues using the VOLUME statement in Windows server 1803?
There was a problem hiding this comment.
Hmm. I did now some more testing and found that USER ContainerAdministrator line actually fixes also this issue.
If that line is not on file and VOLUME statement is starting portainer gives error like this:
2018/07/11 10:30:14 mkdir C:\data: Cannot create a file when that file already exists.
So I will restore that line to file.
1fead33 to
92a16ad
Compare
|
@deviantony yes. Both compose version 2 and swarm stack deployments works like same way like than on 1.18.1. No new bug generated by this changes but I found one Windows containers related bug which already exists on 1.18.1 + TCP (will create old issue from that one). Named Pipe support for Windows containers have been there from day 1 as that it way how docker cli communicates with docker daemon. Feature which we use here is ability mount Named Pipe inside of container and that requires at least Windows 10/Server, version 1709 + Docker 17.09. But older versions can still use old way to communicate over TCP 2375. So this one is ready for merge when winio fix is merged. |
|
Great, let's wait for the merge of microsoft/go-winio#80 then. |
|
Just to confirm, Will the final command look something like this? |
|
@ragaar -u parameter is not need because it is set inside of Dockerfile and you most probably want include data folder mount too like it is on http://portainer.readthedocs.io/en/stable/deployment.html#persist-portainer-data so result will be We hopefully get this one included to release which is scheduled to next week. |
| @@ -1,5 +1,7 @@ | |||
| FROM microsoft/nanoserver | |||
|
|
|||
| USER ContainerAdministrator | |||
There was a problem hiding this comment.
Quick question, will this statement cause any issue with current windows images or 1709 ?
There was a problem hiding this comment.
No. I just quickly tested that docker run -t --rm -u ContainerAdministrator microsoft/nanoserver:1709 cmd /c echo %USERNAME% command works just fine.
There was a problem hiding this comment.
And on microsoft/nanoserver:sac2016 it actually does not change anything as ContainerAdministrator is default setting on that version (but also does not break anything).
There was a problem hiding this comment.
Yes, still fine for 1709 and 1803 windows images.
986fb66 to
f000280
Compare
|
I noticed that this PR was not fully compatible with #2033 so I added one more commit to fix that issue. |
|
microsoft/go-winio#80 has been merged, I'll have a look later today to merge this one. |
|
Thanks to all the people involved ! |
|
@olljanat @StefanScherer one of our user reported the following:
Any pointers on what might be the cause? |
|
@deviantony I'd simplify the command you're running down to minimum inputs required and scale up from there to figure out which command is causing the error. Also, just for reference you defined a data volume but to use it your volume command would look more like this: Full disclosure: I've not done much with volumes in Windows, so I can't confirm the direction of the \ from memory. But that problem, I know, is easy to find online :) |
|
I did not actually execute these commands, someone reported it to us via e-mail :-) I do not have a Windows environment on which I can try to reproduce at the moment. So I'm asking for any help here. |
|
Ahhh! That makes more sense. I don't have Windows either! lol |
|
@deviantony I just tested on Win 10 that both Linux and Windows containers works with commands given on here http://portainer.readthedocs.io/en/stable/deployment.html#windows except that there is small typo which I fixed on portainer/portainer-docs#54 Windows Server 2016 need still follow old documentation: http://portainer.readthedocs.io/en/stable/faq.html#how-can-i-setup-portainer-on-windows-server-2016 |
|
@olljanat as you can see the user is simply running: Which should be working on Win server 2016 |
|
Yep but npipe is not used until user selects local connection. So ask him open new issue with all details. |
|
Ok, I'll ask him to open an issue. Thanks @olljanat |
* apply style to authentication logs and activity logs

Updated PR for which replaces #1186 and solves merge issues on it.
Waiting for microsoft/go-winio#80 to be merged.
Closes #1179
Closes #1728