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
feat(api): Add npipe support #2018
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 ContainerAdministrator I 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -1,6 +1,6 @@ | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can actually remove that comment.
api/http/proxy/manager.go
Outdated
@@ -51,6 +51,9 @@ func (manager *Manager) createDockerProxy(endpointURL *url.URL, tlsConfig *porta | |||
} | |||
return manager.proxyFactory.newDockerHTTPProxy(endpointURL, false), nil | |||
} | |||
if endpointURL.Scheme == "npipe" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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), nil
See 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be renamed newLocalProxy
.
api/http/proxy/factory_windows.go
Outdated
@@ -0,0 +1,33 @@ | |||
// +build windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be renamed newLocalProxy
and it's content should be the actual content of newDockerSocketProxy
(relocate the function here and rename it).
@@ -9,6 +9,57 @@ | |||
<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. My bad.
@@ -54,10 +54,15 @@ function EndpointServiceFactory($q, Endpoints, FileUploadService) { | |||
return Endpoints.remove({id: endpointID}).$promise; | |||
}; | |||
|
|||
service.createLocalEndpoint = function() { | |||
service.createLocalEndpoint = function(Windows) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename the parameter to useNamedPipe
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of adding this ARG layer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the rebase-docker-image
tool still should work for your COPY deployment builds.
build/windows/nanoserver/Dockerfile
Outdated
|
||
VOLUME C:\\data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you choose to remove the default VOLUME statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seal-ss are you aware of any issues using the VOLUME
statement in Windows server 1803?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@StefanScherer thanks for the update and the information, I was not sure about this. Yes, the portainer images embeds the Docker binary in order to manage Swarm stacks. @olljanat Compose stack management on a standalone Docker host still need to be tested, as I am not sure about the usage of npipe with libcompose. |
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question, will this statement cause any issue with current windows images or 1709 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, still fine for 1709 and 1803 windows images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks guys
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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