Windows Container support #96

tmeckel · 2021-03-01T20:16:36Z

SUMMARY

This PR contains changes to docker.py to support Windows Containers.

ISSUE TYPE

Feature Pull Request

…onnection/docker.py

…/connection/docker.py

felixfontein

Thanks for this contribution!

I can't review this fully since I don't have or use Windows; I'll mainly make sure that the code looks good and that it won't break the non-Windows side :-)

Two things:

This needs a changelog fragment;
Please remove the display.debug statements that dump stdout/stderr. This could accidentally dump secrets into logs where they do not belong.

* Added changelog fragment

tmeckel · 2021-03-02T10:58:12Z

@felixfontein Thank for the quick reply on my PR!
I changed the code and added a changelog fragment.

changelogs/fragments/96-windows-support.yaml

plugins/connection/docker.py

Co-authored-by: Felix Fontein <felix@fontein.de>

…ins/connection/docker.py

felixfontein

The non-Windows part of this PR looks good (i.e. it should not break anything ;) ).

tmeckel · 2021-03-02T20:17:05Z

@felixfontein Why are the RHEL 8.2 tasks in the CI pipeline are failing. Is this relevant to my PR?

changelogs/fragments/96-windows-support.yaml

plugins/connection/docker.py

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>

felixfontein · 2021-03-02T20:58:30Z

@tmeckel the RHEL issues are unrelated to this PR, there seems to be something wrong with the setup of the VMs. I've pinged someone to investigate.

felixfontein · 2021-03-02T21:12:19Z

The RHEL will be fixed by #97.

felixfontein · 2021-03-02T21:36:46Z

Tests should now pass. Let's see :)

…s already quoted

felixfontein · 2021-03-04T19:38:43Z

recheck

felixfontein · 2021-03-05T11:49:00Z

@briantist @jborean93 I plan to create a new release of community.docker on Monday or Tuesday next week (so it can get included in Ansible 3.1.0). If you don't manage to review the PR until then, I can also create another release once this is merged, so no pressure ;)

briantist · 2021-03-06T18:06:52Z

I plan to create a new release of community.docker on Monday or Tuesday next week (so it can get included in Ansible 3.1.0). If you don't manage to review the PR until then, I can also create another release once this is merged, so no pressure ;)

I'm not sure I'll be able to actually run this to try it out unfortunately, certainly not before your next release. I'll see if I get a chance to try it out at some point

jborean93

Some nice edge case stuff in the review but what you have here is pretty good.

I will highly recommend that tests be added as soon as possible, there are a few private functions and edge cases with Windows that are hard to keep track off without tests actually testing those scenarios.

plugins/connection/docker.py

jborean93 · 2021-03-07T21:16:18Z

plugins/connection/docker.py

-        local_cmd = self._build_exec_cmd([self._play_context.executable, '-c', cmd])
+        if getattr(self._shell, "_IS_WINDOWS", False):
+            sudoable = False
+            local_cmd = self._build_exec_cmd(["cmd", "/c", cmd])


I might be wrong but I believe using cmd /c here will mean any raw task with the connection plugin will be using cmd as the shell rather than PowerShell. You may want to change this based on whether the ansible_shell_type is either cmd or powershell to allow a user to change this as they desire.

@jborean93 yes this is true. Bit when I change the code as follows I receive a file not found. So the interpreter (Powershell) cannot be found. So I suspect I didn't grasp what you meant by saying I should utilize the ansible_shell_type. Any guidance?

Do you know what the default shell is for a Windows container, i.e. what runs when you do docker exec .... Is it cmd or is it powershell.exe, or maybe no shell at all?

Basically right now if someone was to do the following

- raw: echo "hi"

Is that going to be running under cmd or powershell? With the current changes here that will be running as cmd which might not be ideal if someone has set ansible_shell_type=powershell. Maybe the desire is to have raw with this connection plugin independent to the shell plugin used, if that's the case then the PR as it is makes sense. If you wish for this to be dependent on the shell plugin then you will have to hardcode cmd.exe /c when using the cmd shell and powershell.exe -NoProfile -NonInteractive -ExecutionPolicy Bypass -Command for the powershell shell.

This might be something that @felixfontein may want to comment on.

I don't think I can say something here. I've never used Docker with Windows containers, so I have zero experience what these contain.

I mostly mean more in general, do you expect that raw should match the ansible_shell_type (powershell or cmd) or should it just be the same thing on the connection plugin?

Matching ansible_shell_type would be nice I guess, though I'm not even sure whether Linux containers fully support that (I've never really looked at shell plugins). I also have no idea how complicated it would be to 'properly' implement this.

plugins/connection/docker.py

jborean93 · 2021-03-07T21:22:01Z

plugins/connection/docker.py

@@ -308,11 +330,32 @@ def put_file(self, in_path, out_path):
                count = ' count=0'
            else:
                count = ''
-            args = self._build_exec_cmd([self._play_context.executable, "-c", "dd of=%s bs=%s%s" % (out_path, BUFSIZE, count)])
+            if getattr(self._shell, "_IS_WINDOWS", False):
+                cmd = '''& { $strm = [Console]::OpenStandardInput(1024)


If you use _encode_script as per below you can skip the & { ... } entirely and just run the command as is.

@jborean93 changed the code as advised, but I receive an invalid payload

I'm not sure what is happening here, the error you've shared is something that is thrown by the bootstrap_wrapper.ps1 which is used when executing a module. The code you have here is for copying a file to the docker container and shouldn't be calling the bootstrap_wrapper code.

jborean93 · 2021-03-07T21:43:33Z

plugins/connection/docker.py

+            if getattr(self._shell, "_IS_WINDOWS", False):
+                cmd = '''& { $strm = [Console]::OpenStandardInput(1024)
+$data = [System.Array]::CreateInstance([byte],1024)
+$fstrm = [IO.File]::OpenWrite(%s)


You still need to have more logic to handle here. Right now at a quote glance I could have a path like:

’+ (&[ScriptBlock]::Create([[Text.Encoding]::UTF8.GetString([Convert]::FromBase64String("my code to inject")) +’

Note the use of unicode smart quotes

PowerShell (un)helpfully treats a bunch of smart quotes as the same as single quotes so even if you ran shlex_quote on your path it won't escape what is needed. As soon as you can escape outside of a quote you can embed whatever command you want to run here.

There are 2 choices I can see you moving to:

Use a the same (or similar regex) to the ansible.windows quote filter for powershell to escape the smart quotes and give you a literal value

Base64 encode the path and decode that in your script - base64 means you no longer have to deal with quotes or unicode characters

cmd = ''' ... $outPath = [Text.Encoding]::UTF8.GetString([Convert]::FromBase64String('%s')) ... ''' % (to_text(base64.b64encode(to_bytes(out_path, encoding='utf-8')))) Both options remove the need for `shlex_quote` but I still recommend you normalize the path as you have done today.

felixfontein · 2021-03-11T07:02:30Z

I will highly recommend that tests be added as soon as possible, there are a few private functions and edge cases with Windows that are hard to keep track off without tests actually testing those scenarios.

There's one big problem with Windows container tests: someone has to create and maintain them. Since Windows containers cannot run on Linux, this requires the tests to run on Windows. Our CI (AZP) theoretically does support that, but we've never run tests on Windows for any of the docker plugins and modules yet. So this is far from simple to set up.

jborean93 · 2021-03-11T07:36:09Z

Totally understand that problem, at a quick glance you could do what we did for testing SSH against Windows hosts. Still spin up an AWS EC2 instance for Windows Server 2019 and install the Containers feature and a running container. I'm not sure if it's possible to even expose the Docker port over the internet like that so that might be a no go. Ultimately it's up to you as the maintainers what you would like to have. I just know this is going to be painful to support going forwards without some kind of tests around it.

jimbo8098 · 2021-03-17T12:49:37Z

If this were to be accepted, I have a CI pipeline for my organisation which would make use of these utilities for docker swarm creation and more. We use WinRM and Windows Server 2019 predominantly but resources to work with others since idempotent IasC is in it's infancy here.

I was actually on my way to find out the views of me PRing the community.docker.docker_swarm module to support Windows!

Therefore, watching this and stand ready to assist.

felixfontein · 2021-03-17T13:00:17Z

@jimbo8098 I'm happy to merge this (once all review comments are addressed) also without Windows tests. We (or in particularly I :-) ) don't mind Windows-specific PRs - as long as they don't break other platforms.

It would be great to have Windows tests in this repository as well; as long as we don't have them, Windows support in the modules is "use on your own risk, we try not to break it, but we won't notice in case we do". If your CI pipeline uses say the main branch of this collection, you would notice early if something breaks and you could create an issue (or even a PR to fix it), so that releases should usually work for Windows :)

jimbo8098 · 2021-03-17T13:02:37Z

Just to add to my previous point, Our infrastructure is Windows Server 2019 hardware, not containers. I suspect the two should be the same in a CI environment if you are just building towards testing infra for this though some mechanisms of course might be a bit different. Will look to PR on docker_swarm for the moment but I'll be watching this.

felixfontein · 2021-04-05T10:16:49Z

@tmeckel do you have time to incorporate the latest review comments? It would be great if we could get this merged.

tmeckel · 2021-04-17T19:57:44Z

@felixfontein sorry for the delay. Found some time to work on the comments of @jborean93

felixfontein · 2021-05-04T06:12:17Z

@tmeckel ping

tmeckel · 2021-05-05T19:48:31Z

@felixfontein added comments. Awaiting answers from @jborean93

github-actions · 2023-11-03T07:37:26Z

Docs Build 📝

This PR is closed and any previously published docsite has been unpublished.

tmeckel added 2 commits March 1, 2021 20:54

Added changes to support working with Windows Containers to plugins/c…

57ca05e

…onnection/docker.py

Commit two regressions in documentation and transport name in plugins…

6bdd663

…/connection/docker.py

This was referenced Mar 1, 2021

Fix to support Windows Containers with win_package ansible-collections/ansible.windows#189

Closed

Fix to support logging into a Windows Container ansible-community/molecule-docker#40

Closed

felixfontein reviewed Mar 1, 2021

View reviewed changes

* Removed display.debug statements

6e00312

* Added changelog fragment

felixfontein reviewed Mar 2, 2021

View reviewed changes

changelogs/fragments/96-windows-support.yaml Outdated Show resolved Hide resolved

plugins/connection/docker.py Outdated Show resolved Hide resolved

plugins/connection/docker.py Outdated Show resolved Hide resolved

tmeckel and others added 2 commits March 2, 2021 19:45

Update changelogs/fragments/96-windows-support.yaml

b8be89e

Co-authored-by: Felix Fontein <felix@fontein.de>

Removed self._prefix_login_path and shlex_quote from put_file in plug…

c093c94

…ins/connection/docker.py

felixfontein requested a review from jborean93 March 2, 2021 19:30

felixfontein approved these changes Mar 2, 2021

View reviewed changes

briantist reviewed Mar 2, 2021

View reviewed changes

changelogs/fragments/96-windows-support.yaml Outdated Show resolved Hide resolved

plugins/connection/docker.py Outdated Show resolved Hide resolved

Update plugins/connection/docker.py

3dfef15

Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>

Update changelogs/fragments/96-windows-support.yaml

2709aae

Corrected Powershell command in put_file function, because out_path i…

eda7ab2

…s already quoted

jborean93 reviewed Mar 7, 2021

View reviewed changes

jimbo8098 mentioned this pull request Mar 17, 2021

docker_swarm Windows Support #106

Closed

felixfontein mentioned this pull request Apr 11, 2021

Release plan #4

Open

felixfontein added the docker-plain plain Docker (no swarm, no compose, no stack) label May 21, 2021

tmeckel closed this Nov 3, 2023

tmeckel deleted the windows-support branch November 3, 2023 07:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Container support #96

Windows Container support #96

tmeckel commented Mar 1, 2021

felixfontein left a comment

tmeckel commented Mar 2, 2021

felixfontein left a comment

tmeckel commented Mar 2, 2021

felixfontein commented Mar 2, 2021

felixfontein commented Mar 2, 2021

felixfontein commented Mar 2, 2021

felixfontein commented Mar 4, 2021

felixfontein commented Mar 5, 2021

briantist commented Mar 6, 2021

jborean93 left a comment

jborean93 Mar 7, 2021

tmeckel May 5, 2021 •

edited

Loading

jborean93 May 11, 2021

felixfontein May 17, 2021

jborean93 May 17, 2021

felixfontein May 17, 2021

jborean93 Mar 7, 2021

tmeckel May 5, 2021

jborean93 May 11, 2021

jborean93 Mar 7, 2021

felixfontein commented Mar 11, 2021

jborean93 commented Mar 11, 2021

jimbo8098 commented Mar 17, 2021 •

edited

Loading

felixfontein commented Mar 17, 2021

jimbo8098 commented Mar 17, 2021

felixfontein commented Apr 5, 2021

tmeckel commented Apr 17, 2021

felixfontein commented May 4, 2021

tmeckel commented May 5, 2021

github-actions bot commented Nov 3, 2023

Windows Container support #96

Windows Container support #96

Conversation

tmeckel commented Mar 1, 2021

SUMMARY

ISSUE TYPE

felixfontein left a comment

Choose a reason for hiding this comment

tmeckel commented Mar 2, 2021

felixfontein left a comment

Choose a reason for hiding this comment

tmeckel commented Mar 2, 2021

felixfontein commented Mar 2, 2021

felixfontein commented Mar 2, 2021

felixfontein commented Mar 2, 2021

felixfontein commented Mar 4, 2021

felixfontein commented Mar 5, 2021

briantist commented Mar 6, 2021

jborean93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeckel May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixfontein commented Mar 11, 2021

jborean93 commented Mar 11, 2021

jimbo8098 commented Mar 17, 2021 • edited Loading

felixfontein commented Mar 17, 2021

jimbo8098 commented Mar 17, 2021

felixfontein commented Apr 5, 2021

tmeckel commented Apr 17, 2021

felixfontein commented May 4, 2021

tmeckel commented May 5, 2021

github-actions bot commented Nov 3, 2023

Docs Build 📝

tmeckel May 5, 2021 •

edited

Loading

jimbo8098 commented Mar 17, 2021 •

edited

Loading