docker: improve TLS config #53906

felixfontein · 2019-03-16T17:41:41Z

SUMMARY

This PR does several things:

unify TLS setup for inventory plugin and docker modules (not done yet)
make tls and tls_verify options no longer mutually exclusive, and let tls_verify (host verification) take precedence over tls (no host verification) in case both are set to yes.

This also fixes #15614 and makes #51271 no longer necessary.

ISSUE TYPE

Bugfix Pull Request
Feature Pull Request

COMPONENT NAME

lib/ansible/module_utils/docker/common.py
lib/ansible/plugins/doc_fragments/docker.py
lib/ansible/plugins/inventory/docker_swarm.py

ansibot · 2019-03-16T17:47:35Z

cc @DBendit @WojciechowskiPiotr @akshay196 @chouseknecht @cove @danihodovic @dariko @dusdanig @joshuaconner @jwitko @kassiansun @softzilla @tbouvet @zfil
click here for bot help

felixfontein · 2019-03-16T20:19:38Z

bot_status

ansibot · 2019-03-16T20:26:33Z

Components

lib/ansible/module_utils/docker/common.py
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

lib/ansible/modules/cloud/docker/docker_container.py
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 chouseknecht cove danihodovic dariko dusdanig felixfontein joshuaconner jwitko kassiansun softzilla tbouvet zfil

lib/ansible/plugins/doc_fragments/docker.py
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

lib/ansible/plugins/inventory/docker_swarm.py
support: community
maintainers: DBendit WojciechowskiPiotr akshay196 danihodovic dariko felixfontein jwitko kassiansun tbouvet

Metadata

waiting_on: maintainer
changes_requested_by: null
needs_info: False
needs_revision: False
needs_rebase: False
merge_commits: []
too many files or commits: False
mergeable_state: unstable
shippable_status: pending
maintainer_shipits (module maintainers): False
community_shipits (namespace maintainers): False
ansible_shipits (core team members): False
shipit_actors (maintainer or core team member): None
shipit_actors_other:
automerge: automerge shipit test failed

click here for bot help

felixfontein · 2019-03-16T20:28:47Z

Looks like either ansibot didn't update its metadata, or something else doesn't work so that @morph027 isn't notified.

morph027 · 2019-03-16T20:33:53Z

Hah, now i'am ;) Will look into it.

…ory plugin.

…plugin.

morph027 · 2019-03-17T14:49:19Z

Besides my 2 little findings, i just tested successfully w/ and w/o TLS 🎉

morph027 · 2019-03-17T14:42:21Z

lib/ansible/plugins/inventory/docker_swarm.py

@@ -29,7 +29,9 @@
            required: true
            choices: docker_swarm
        host:


needs to be renamed to docker_host or we'll get this warning:

[WARNING]: * Failed to parse /home/morph/ansible/docker_swarm.yml with auto plugin: 'Requested entry (plugin_type: inventory plugin: docker_swarm setting: docker_host ) was not defined in configuration.'

Applies to examples too

Done. It would also be enough to fix line 151, but I think docker_host is somewhat better than host since it's the same name for the docker_* modules.

morph027 · 2019-03-17T14:46:16Z

lib/ansible/plugins/inventory/docker_swarm.py

+                raise AnsibleError('Argument to timeout function must be an integer')
+        update_tls_hostname(raw_params)
+        connect_params = get_connect_params(raw_params, fail_function=self._fail)
+        self.client = docker.DockerClient(**get_connect_params)


Not self.client = docker.DockerClient(**connect_params) ?

Yeah, good spot! Fixed.

WojciechowskiPiotr · 2019-03-17T15:25:07Z

make tls and tls_verify options no longer mutually exclusive, and let tls_verify (host verification) take precedence over tls (no host verification) in case both are set to yes.

Just to confirm - if you provide only the tls_verify or both the tls_verify and tls it will run in tls_verify mode?

WojciechowskiPiotr · 2019-03-17T15:26:48Z

lib/ansible/module_utils/docker/common.py

+
+def get_connect_params(auth, fail_function):
+    if auth['tls'] or auth['tls_verify']:
+        auth['docker_host'] = auth['docker_host'].replace('tcp://', 'https://')


I would drop it. Let the docker library handle this if tcp:// is provided. The kwargs_from_env from docker.utils handles that, also parse_host method.

I would keep it for this PR, and remove it in another PR, so that this refactoring doesn't change any behavior. Just in case this has some impact somewhere, it can be undone quickly without reverting everything from this PR :)

(Also, we're not using kwargs_from_env.)

No problem, if then all integration tests are still good we may leave it to docker library to change it or not

All failing integration tests are unrelated (installing packages for Fedora fails a lot), at least so far. So it probably is fine :) Could you test whether you can still connect to your swarm with TLS both with the inventory plugin and some of the docker_* modules? If yes, I'd like to get this merged soon (especially since every CI run takes foreeeever and has so many unrelated problems :) ).

felixfontein · 2019-03-17T15:51:50Z

Just to confirm - if you provide only the tls_verify or both the tls_verify and tls it will run in tls_verify mode?

Yes (assuming they are set to yes). If any one of them is set to yes, TLS is enabled, and if tls_verify is set to yes, verification will be used.

felixfontein · 2019-03-17T15:56:03Z

Did you test both the inventory plugin and the regular docker_* modules (I guess one suffices, since they all use the same setup code) with TLS?

morph027 · 2019-03-17T17:59:08Z

I only tested the inventory plugin....did not yet used the others.

WojciechowskiPiotr · 2019-03-17T18:04:56Z

I will test later today the modules

WojciechowskiPiotr · 2019-03-17T18:52:06Z

Tested docker_swarm_facts and docker_node_facts - both working fine but I use only the tls parameter at the moment

felixfontein · 2019-03-17T19:35:42Z

Does one of you have a chance to test this with TLS verification? If not, we can probably just merge now. The part of the code which decides between verification and not looks "harmless" enough so that should still work (assuming it ever worked, that is :) ).

morph027 · 2019-03-17T19:38:10Z

Will do.

morph027 · 2019-03-17T19:46:07Z

TLS with client cert auth works 👍

felixfontein · 2019-03-17T19:52:16Z

Awesome! I'll try to get this merged soon. Once the tests pass...

felixfontein · 2019-03-17T20:45:45Z

shipit

WojciechowskiPiotr · 2019-03-17T21:01:07Z

shipit

felixfontein · 2019-03-17T21:02:53Z

ansibot is busy processing old issues/PRs, it will take some more time until it will reprocess this PR...

felixfontein · 2019-03-17T21:12:36Z

That was faster than I hoped. Good for us :D

@WojciechowskiPiotr @morph027 thanks a lot for reviewing and testing!

felixfontein mentioned this pull request Mar 16, 2019

Avoid docker TLS and TLS_VERIFY exclusive error #51271

Closed

felixfontein force-pushed the docker-tls-config branch from 1f787f7 to 611cca9 Compare March 16, 2019 18:13

felixfontein mentioned this pull request Mar 16, 2019

docker_swarm inventory plugin - new attribute with parsed node URI #53894

Merged

felixfontein mentioned this pull request Mar 16, 2019

New module docker_plugin: Install/remove Docker plugins #52643

Closed

felixfontein changed the title ~~[WIP] docker: improve TLS config~~ docker: improve TLS config Mar 16, 2019

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Mar 16, 2019

felixfontein force-pushed the docker-tls-config branch from 7d1d6fd to b9fe410 Compare March 16, 2019 20:37

felixfontein added 5 commits March 17, 2019 10:58

Stop repeating names of common config arguments in docker_container.

72cb12a

Prefer tls_verify over tls for docker modules and docker_swarm invent…

c1b35dd

…ory plugin.

tls and tls_verify are no longer mutually exclusive.

2e463e8

Share setup code between docker_* modules and docker_swarm inventory …

72e4955

…plugin.

Add support for more parameters.

4860a0d

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 17, 2019

morph027 suggested changes Mar 17, 2019

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 17, 2019

ansibot removed the needs_triage Needs a first human triage before being processed. label Mar 17, 2019

WojciechowskiPiotr reviewed Mar 17, 2019

View reviewed changes

felixfontein added 2 commits March 17, 2019 16:52

Fix typo.

8811c92

Rename host -> docker_host.

bf4ed13

ansibot added automerge This PR was automatically merged by ansibot. shipit This PR is ready to be merged by Core and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 17, 2019

ansibot merged commit 29d6418 into ansible:devel Mar 17, 2019

felixfontein deleted the docker-tls-config branch March 17, 2019 21:12

ansible locked and limited conversation to collaborators Jul 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docker: improve TLS config #53906

docker: improve TLS config #53906

felixfontein commented Mar 16, 2019

ansibot commented Mar 16, 2019

felixfontein commented Mar 16, 2019

ansibot commented Mar 16, 2019

felixfontein commented Mar 16, 2019

morph027 commented Mar 16, 2019

morph027 commented Mar 17, 2019

morph027 Mar 17, 2019

felixfontein Mar 17, 2019

morph027 Mar 17, 2019

felixfontein Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

WojciechowskiPiotr Mar 17, 2019

felixfontein Mar 17, 2019

WojciechowskiPiotr Mar 17, 2019

felixfontein Mar 17, 2019

felixfontein commented Mar 17, 2019

felixfontein commented Mar 17, 2019

morph027 commented Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

felixfontein commented Mar 17, 2019

morph027 commented Mar 17, 2019

morph027 commented Mar 17, 2019

felixfontein commented Mar 17, 2019

felixfontein commented Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

felixfontein commented Mar 17, 2019

felixfontein commented Mar 17, 2019

docker: improve TLS config #53906

docker: improve TLS config #53906

Conversation

felixfontein commented Mar 16, 2019

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibot commented Mar 16, 2019

felixfontein commented Mar 16, 2019

ansibot commented Mar 16, 2019

felixfontein commented Mar 16, 2019

morph027 commented Mar 16, 2019

morph027 commented Mar 17, 2019

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

WojciechowskiPiotr commented Mar 17, 2019

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 17, 2019

felixfontein commented Mar 17, 2019

morph027 commented Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

felixfontein commented Mar 17, 2019

morph027 commented Mar 17, 2019

morph027 commented Mar 17, 2019

felixfontein commented Mar 17, 2019

felixfontein commented Mar 17, 2019

WojciechowskiPiotr commented Mar 17, 2019

felixfontein commented Mar 17, 2019

felixfontein commented Mar 17, 2019