feat: enable connecting to rootfull pods #313
Conversation
|
I saw some tests in |
plugins/connection/podman.py
Outdated
| @@ -68,6 +70,20 @@ | |||
| - name: ansible_podman_executable | |||
| env: | |||
| - name: ANSIBLE_PODMAN_EXECUTABLE | |||
| podman_rootless: | |||
| description: Whether the container should be run as root. | |||
There was a problem hiding this comment.
I think you meant "should be run as rootless"?
plugins/connection/podman.py
Outdated
| if not podman_cmd: | ||
| raise AnsibleError("%s command not found in PATH" % podman_exec) | ||
| local_cmd = [podman_cmd] | ||
| elif not sudo_cmd: |
There was a problem hiding this comment.
This check makes sense only if "root" connection is set. We don't want to fail all cases where there is no sudo in the host.
plugins/connection/podman.py
Outdated
| @@ -102,11 +145,17 @@ def _podman(self, cmd, cmd_args=None, in_data=None, use_container_id=True): | |||
| :param in_data: data passed to podman's stdin | |||
| :return: return code, stdout, stderr | |||
| """ | |||
| rootless = self.get_option('podman_rootless') | |||
There was a problem hiding this comment.
By default we need to follow the current user behavior. This setting makes sense only when current user is not root and rootless is false. I think this check we should do here.
Otherwise when this module is running from root user you have to set rootless to false every time, and this is not convenient.
| podman_exec = self.get_option('podman_executable') | ||
| podman_cmd = distutils.spawn.find_executable(podman_exec) | ||
| sudo_cmd = distutils.spawn.find_executable('sudo') |
There was a problem hiding this comment.
This check makes sense only if "root" connection is set. We don't want to fail all cases where there is no sudo in the host.
| elif not sudo_cmd: | ||
| raise AnsibleError("sudo command not found in PATH") | ||
|
|
||
| local_cmd = [podman_cmd] if rootless else [sudo_cmd, '-Sk', podman_cmd] |
There was a problem hiding this comment.
Like I mentioned before, the default behavior should be for current user.
Probably it's better to set setting like root_connection, not rootless
|
Hi @sshnaidm thank you for your review, I indeed did not think of the case where the user is already root. I proposed a new version that resolve a few of your feedbacks but I agree that it's not a perfect fix. I'll try to provide a better solution in the following days but I'm not sure I understand your idea with the |
Fixing #300.
It only works with
sudofor now.