Skip to content
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

Additionally mount ~/.ssh/ to /root/.ssh inside EEs #805

Closed
wants to merge 0 commits into from

Conversation

shanemcd
Copy link
Member

Currently when running inside an EE as root, only SSH_AUTH_SOCK is working.

This enables the usage of default key names under ~/.ssh/on the host.

@shanemcd shanemcd requested a review from a team as a code owner August 27, 2021 15:34
@cidrblock
Copy link
Contributor

cidrblock commented Aug 27, 2021

Some background:

paramiko looks for keys in $HOME/.ssh, $HOME in the EE is set to /home/runner
openssh pulls home from /etc/passwd, which is /root

this change will allow for both the use of paramiko and openssh within the EE, and keys being located in each of those default search paths

(this all w/o ssh-agent)

@Shrews
Copy link
Contributor

Shrews commented Aug 30, 2021

LGTM, except for needing test updates.

Copy link
Contributor

@samdoran samdoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good. The tests are failing due to a mismatch in expected order of options:

test/unit/config/test_ansible_cfg.py:92: AssertionError

AssertionError: assert '-v' == '--group-add=root'

Do we need to add new tests for this to ensure it doesn't break in the future?

ansible-zuul bot added a commit that referenced this pull request Sep 14, 2021
Fix unit tests to run consistently in CI and locally

use pytest-mock and tmp_path fixtures
mock test environment variables to prevent inconsistent test results
reformat test fixtures so they are easier to read

This will resolve the test failures in #805.

Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
Reviewed-by: Sam Doran <sdoran@redhat.com>
Reviewed-by: Alan Rominger <arominge@redhat.com>
Reviewed-by: David Shrewsbury <None>
Reviewed-by: None <None>
@samdoran
Copy link
Contributor

recheck

@samdoran samdoran changed the title Additionally mount ~/.ssh/ to /root/.ssh inside EEs Additionally mount ~/.ssh/ to /root/.ssh inside EEs Sep 15, 2021
@samdoran samdoran changed the title Additionally mount ~/.ssh/ to /root/.ssh inside EEs Additionally mount ~/.ssh/ to /root/.ssh inside EEs Sep 15, 2021
@shanemcd shanemcd closed this Sep 15, 2021
@samdoran
Copy link
Contributor

Do we not need this anymore? It looks like the tests will need to be updated since a new mount is being added. I plan to refactor the command and object generation into common functions that can be shared. That will minimize the locations we need to change the tests when the code changes.

ansible-zuul bot added a commit that referenced this pull request Sep 17, 2021
Additionally mount ~/.ssh/ to /root/.ssh inside EEs

I accidentally closed #805 with a force-push and can't figure out how to reopen.
Currently when running inside an EE as root, only SSH_AUTH_SOCK is working. This enables the usage of default key names under ~/.ssh/ on the host.

Reviewed-by: David Shrewsbury <None>
Reviewed-by: None <None>
samdoran pushed a commit to samdoran/ansible-runner that referenced this pull request Oct 14, 2021
Fix unit tests to run consistently in CI and locally

use pytest-mock and tmp_path fixtures
mock test environment variables to prevent inconsistent test results
reformat test fixtures so they are easier to read

This will resolve the test failures in ansible#805.

Reviewed-by: Sviatoslav Sydorenko <webknjaz+github/profile@redhat.com>
Reviewed-by: Sam Doran <sdoran@redhat.com>
Reviewed-by: Alan Rominger <arominge@redhat.com>
Reviewed-by: David Shrewsbury <None>
Reviewed-by: None <None>
(cherry picked from commit 2323686)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants