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

Implement connection plugin for podman #47519

Merged
merged 3 commits into from Nov 1, 2018

Conversation

Projects
None yet
5 participants
@TomasTomecek
Contributor

TomasTomecek commented Oct 23, 2018

SUMMARY

This is a new connection plugin for podman container manager.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

connection_podman

ADDITIONAL INFORMATION

Please see test_connection.inventory file for instructions how to test this.

The reason why this is WIP is that when I change ansible_remote_user to 1000, ansible figures out the out_path to ~1000. I'm actually not sure where the problem is.

<podman-container> PUT /home/tt/.ansible/tmp/ansible-local-6366fTcbAW/tmpuYEzv5 TO ~1000/.ansible/tmp/ansible-tmp-1540310223.97-201232050279128/AnsiballZ_file.py

I'm also not interested in changing the posix.sh file, this is just current iteration how I tried to debug and fix stuff.

CC @rhatdan @baude

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 23, 2018

Hi @TomasTomecek, thank you for submitting this pull-request!

click here for bot help

@sivel

This comment has been minimized.

Member

sivel commented Oct 23, 2018

The reason why this is WIP is that when I change ansible_remote_user to 1000, ansible figures out the out_path to ~1000. I'm actually not sure where the problem is.

I believe this is due to how we expand remote paths:

if expand_path == '~':
# Network connection plugins (network_cli, netconf, etc.) execute on the controller, rather than the remote host.
# As such, we want to avoid using remote_user for paths as remote_user may not line up with the local user
# This is a hack and should be solved by more intelligent handling of remote_tmp in 2.7
if getattr(self._connection, '_remote_is_local', False):
pass
elif sudoable and self._play_context.become and self._play_context.become_user:
expand_path = '~%s' % self._play_context.become_user
else:
# use remote user instead, if none set default to current user
expand_path = '~%s' % (self._play_context.remote_user or self._connection.default_user or '')

We effectively run echo ~1000 where 1000 would be a username that is present on the target system.

If 1000 was a username on the remote system, and assuming the home dir was set to /home/1000 that should return /home/1000 instead.

Based on what you have found, it appears as though there is no user with a username of 1000 on the remote host. As such you end up with a literal ~1000

@sivel sivel removed the needs_triage label Oct 23, 2018

@samdoran samdoran self-assigned this Oct 23, 2018

@sivel

This comment has been minimized.

Member

sivel commented Oct 23, 2018

I'm guessing by the fact that the user is 1000 that it is a UID and not a username. Which would be the problem.

@samdoran

This comment has been minimized.

Member

samdoran commented Oct 23, 2018

@TomasTomecek Thanks for submitting this. I'm working on a suite of Podman modules for TripleO, and this connection plugin was on my list.

I wrote podman_image_facts and am working on podman_image. Let me know if this is of interest to you since we may upstream these in Ansible once they are complete.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 23, 2018

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/connection_podman/aliases:0:0: missing alias `shippable/posix/group[1-3]` or `unsupported`

click here for bot help

@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Oct 24, 2018

Matt, thank you, that's so helpful!

Unfortunately that doesn't work well for containers. The thing is that it's a fairly normal use case to run a command in a container under a user which is not defined in /etc/passwd:

$ podman run -d --name hi busybox sleep 99999
bf639cb1daee8888a319021a1f0d085ddf5611bcdd90f89cd6da6972d4093d85
$ podman exec hi id                                                                                                                                              
uid=0(root) gid=0(root) groups=65534(nogroup),0(root),65534(nogroup)
$ podman exec -u 1234 hi id                                                                                                                                      
uid=1234 gid=0(root) groups=65534(nogroup),0(root),65534(nogroup)

I'm not sure if we can change that logic or whether I should handle such case inside the connection plugin.

Sam, thanks for update! I'll definitely take a look. Maybe to give you some context, I wrote buildah connection plugin back then for ansible-container. Doing podman now pretty much for the same reason.

Edit: playing with this right now and the other problem is this command:

<podman-container> RUN ['podman', 'exec', 'podman-container', '/bin/sh', '-c', '( umask 77 && mkdir -p "` echo ~1000/.ansible/tmp/ansible-tmp-1540379927.41-217603405047264 `
" && echo ansible-tmp-1540379927.41-217603405047264="` echo ~1000/.ansible/tmp/ansible-tmp-1540379927.41-217603405047264 `" ) && sleep 0', '--user', '1000']  

Again, we would need to change the script so it would work with containers. Therefore I'm dropping support for remote_user for now.

@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Oct 24, 2018

Just pushed the initial support without remote_user. Keeping en eye on CI.

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 24, 2018

The test ansible-test sanity --test integration-aliases [explain] failed with 1 error:

test/integration/targets/connection_podman/aliases:0:0: missing alias `shippable/posix/group[1-3]` or `unsupported`

The test ansible-test sanity --test no-underscore-variable [explain] failed with 1 error:

lib/ansible/plugins/connection/podman.py:120:15: use `dummy` instead of `_` for a variable name

click here for bot help

@ansibot ansibot added the ci_verified label Oct 24, 2018

new connection plugin: podman
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>

@ansibot ansibot removed the ci_verified label Oct 24, 2018

@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Oct 24, 2018

All is green and nice now! Please let me know what I should do to get this merged.

@sivel

This comment has been minimized.

Member

sivel commented Oct 24, 2018

I'm not sure if we can change that logic or whether I should handle such case inside the connection plugin.

The logic for expanding the remote path has gone through many iterations to cover as many edge cases as possible. What we have currently we believe is the most flexible.

However, you might add a note with this connection plugin that ansible_remote_tmp (host or group var) should be set for hosts using this connection plugin to use a path not dependent on the users home directory such as /tmp when the user you are running the command as does not exist on the system.

I don't know much about podman, but fwiw, we already support various container types as connection plugins, and we haven't run into your scenario before.

As far as handling it in the connection, I don't think that can be easily influenced from the connection plugin, since the command building is handled in the action plugin. I don't think we would accept code that tried to parse those commands and modify them from the connection.

@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Oct 25, 2018

@sivel, you're a genius!

Setting ansible_remote_user indeed did the trick. To be honest, I tried that myself and I actually expected that this line does it:

https://github.com/ansible/ansible/blob/devel/test/integration/targets/connection_posix/test.sh#L17

Unfortunately it didn't.

@TomasTomecek TomasTomecek changed the title from WIP: Implement connection plugin for podman to Implement connection plugin for podman Oct 25, 2018

@ansibot ansibot added core_review and removed WIP labels Oct 25, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 25, 2018

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/plugins/connection/podman.py:46:100: W291 trailing whitespace

click here for bot help

podman,conn: utilize remote_user to run commands
Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>
@rhatdan

This comment has been minimized.

rhatdan commented Oct 25, 2018

@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Oct 26, 2018

@sivel @samdoran All is green, shall we merge?

I'll write down a quick blog post once this is merged. Can I expect this to be in 2.8?

@samdoran

I wanted to do some testing with it first. Seems to work very well. Thank you for writing this (and for including tests). Just a few minor changes to the documentation.

Show resolved Hide resolved lib/ansible/plugins/connection/podman.py Outdated
Show resolved Hide resolved lib/ansible/plugins/connection/podman.py Outdated
Show resolved Hide resolved lib/ansible/plugins/connection/podman.py Outdated
Show resolved Hide resolved lib/ansible/plugins/connection/podman.py Outdated

@ansibot ansibot added needs_revision and removed core_review labels Oct 29, 2018

podman connection: update docs
Co-Authored-By: TomasTomecek <ttomecek@redhat.com>
@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Oct 30, 2018

Thanks Sam, very good suggestions. I squashed them into a single commit.

@samdoran samdoran merged commit 23becec into ansible:devel Nov 1, 2018

1 check passed

Shippable Run 90948 status is SUCCESS.
Details

@TomasTomecek TomasTomecek deleted the TomasTomecek:podman-conn branch Nov 1, 2018

@TomasTomecek

This comment has been minimized.

Contributor

TomasTomecek commented Nov 1, 2018

So stoked to have this!! Gonna write that blog post so that people know how to use this.

Sam and Matt, thanks a lot for your help!

Update: here it is: https://blog.tomecek.net/post/ansible-and-podman-can-play-together-now/

jeking3 added a commit to jeking3/ansible that referenced this pull request Nov 6, 2018

Implement connection plugin for podman (ansible#47519)
* new connection plugin: podman

Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>

* podman,conn: utilize remote_user to run commands

Signed-off-by: Tomas Tomecek <ttomecek@redhat.com>

* podman connection: update docs

Co-Authored-By: TomasTomecek <ttomecek@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment