-
Notifications
You must be signed in to change notification settings - Fork 359
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
Fix image build to override root homedir in /etc/passwd #1027
Conversation
Build succeeded.
|
argh, looks like we'll also need to fix this hack: https://github.com/ansible/ansible-runner/blob/devel/utils/entrypoint.sh#L14-L15 |
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 agree that this is a serious bug but at the same time, I doubt that changing room home directory to make it use another folder is ever a good idea.
I do see two paths here: switch to non root user or go for using /root
.
I'd like @shanemcd to be looped into this |
So would I- I'd love to know the history on this, since the commit history on the container definition isn't telling me much other than "we've dorked around with this an awful lot". |
I am curious about the history too but not for blaming reasons. To be honest I would prefer to go for a standard setup: either: a) root user with its default home directory Black magic is never a good idea, can hit you back when you expect the least. |
My own understanding is that there are two requirements which are challenging to satisfy simultaneous (1) rootless podman usage and (2) ability to use in OCP. I would like to note that integration tests are already failing. I would like to have more integration testing with more of the platform, as this is the type of change where we would like to test as much as possible in advance. |
The primary point of complexity here is supporting arbitrary UIDs. We need to support this for 2 main use cases:
In response to the changes made so far in this PR: If we remove
This causes container group jobs to fail with an error like:
One alternative would be to remove As awful as it looks, I dont think there would be an issue with changing the root user's home directory as long as |
Tried that here and it seems to work: https://github.com/shanemcd/ansible-runner/pull/2/checks (The failing test was quay.io throwing a 500 error.) |
I think this should now cover the various cases- still some cleanup to do, it's still spewing noise on startup because I suck at bash, but I think the overall approach should ensure that we've always got a workable homedir that's consistent everywhere, whether it's a dynamic uid or a real user. |
Build succeeded.
|
In addition to the noise at startup, the tests are also failing because of how we mount runner's display plugin into the container from the host. This code was also written with the assumption that HOME is always
|
If we want to keep trying to go down this route, I think it would work if we just changed the code to mount the callback under both |
Actually... we can just do this: diff --git a/ansible_runner/utils/__init__.py b/ansible_runner/utils/__init__.py
index 214b0a7..c8869e2 100644
--- a/ansible_runner/utils/__init__.py
+++ b/ansible_runner/utils/__init__.py
@@ -68,7 +68,6 @@ def callback_mount(copy_if_needed=False):
if copy_if_needed is set, and the install is owned by another user,
it will copy the plugin to a tmpdir for the mount in anticipation of SELinux problems
'''
- container_dot_ansible = '/home/runner/.ansible'
rel_path = ('callback', '',)
host_path = os.path.join(get_plugin_dir(), *rel_path)
if copy_if_needed:
@@ -78,7 +77,7 @@ def callback_mount(copy_if_needed=False):
register_for_cleanup(tmp_path)
host_path = os.path.join(tmp_path, 'callback')
shutil.copytree(callback_dir, host_path)
- container_path = os.path.join(container_dot_ansible, 'plugins', *rel_path)
+ container_path = os.path.join('/usr/share/ansible', 'plugins', *rel_path)
return (host_path, container_path) |
After trying my proposed change locally, some of the tests are now intermittently failing with:
This might be a race condition because we are using the ansible-runner/ansible_runner/config/_base.py Line 502 in 1ab1ed5
From
|
I've confirmed that the tests pass when changing So now, my current concern here is that these latest findings will not only require a change at the EE level, but would also require changes in ansible-runner itself. This would prevent users from being able to use the newer EEs with the versions of ansible-runner 2.x already out in the wild. Unless there is a valid reason about why the more surgical solution I proposed above will not work, I do not think this warrants all of the cascading work that will fall out of making this change. |
One way or another, we still have to fix the HOME/passwd mismatches by the time Ansible runs (which your change alone doesn't)- the current test suite doesn't have any tests that reflect the failure we're actually trying to fix. But yeah, I definitely want to limit the blast radius of whatever this fix ends up looking like to avoid changing the outer runner's volume mount behavior, because IIUC we don't currently have a good way to "sniff" the inner runner version or validate that the assumptions each will make are compatible (something else I have some ideas about- we have to deal with that soon). There might be some latent weirdness in certain This also works with the |
Now that https://github.com/ansible-community/toolset is deprecated, more and more people tend to turn towards https://github.com/ansible/creator-ee, which builds on top of ansible-runner. More issues related to #1024 and this PR are raised. Is there anything that can be done to move things forward? |
Hey, can I help with with bringing this forward?:) |
Build succeeded.
|
Build succeeded.
|
@shanemcd the only additional change I can think of right now that might be needed here is to restore the |
* using only `HOME` envvar to override homedir causes mismatches with anything that asks for it a different way (eg, `echo ~root`) * kicking the can down the road on "why are we overriding /root as root's homedir anyway?"
* override root homedir in /etc/passwd * using only `HOME` envvar to override homedir causes mismatches with anything that asks for it a different way (eg, `echo ~root`) * kicking the can down the road on "why are we overriding /root as root's homedir anyway?" * more dynamic homedir handling * just make everybody's home /home/runner * restore ENV HOME override for builds * comment wording (cherry picked from commit c181daa)
* override root homedir in /etc/passwd * using only `HOME` envvar to override homedir causes mismatches with anything that asks for it a different way (eg, `echo ~root`) * kicking the can down the road on "why are we overriding /root as root's homedir anyway?" * more dynamic homedir handling * just make everybody's home /home/runner * restore ENV HOME override for builds * comment wording (cherry picked from commit c181daa)
* override root homedir in /etc/passwd * using only `HOME` envvar to override homedir causes mismatches with anything that asks for it a different way (eg, `echo ~root`) * kicking the can down the road on "why are we overriding /root as root's homedir anyway?" * more dynamic homedir handling * just make everybody's home /home/runner * restore ENV HOME override for builds * comment wording (cherry picked from commit c181daa)
* override root homedir in /etc/passwd * using only `HOME` envvar to override homedir causes mismatches with anything that asks for it a different way (eg, `echo ~root`) * kicking the can down the road on "why are we overriding /root as root's homedir anyway?" * more dynamic homedir handling * just make everybody's home /home/runner * restore ENV HOME override for builds * comment wording (cherry picked from commit c181daa)
fixes #1024
HOME
envvar to override homedir causes mismatches with anything that asks for it a different way (eg,echo ~root
)