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

lxd_connection: Allow non-root users to connect to an instance #9659

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

yeetypete
Copy link

@yeetypete yeetypete commented Jan 31, 2025

SUMMARY

Currently the lxd_connection only supports connecting to an instance as root. This PR extends the plugin to allow a non-root user, configurable via the ansible_user var to connect to the instance. The option lxd_become_method controls the command used to switch users, (su by default but could also be sudo -u). The defaults ensure the old behavior so this should be a non-breaking change.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lxd_connection

ADDITIONAL INFORMATION

If it is beneficial I would also be happy to add this functionality to the incus_connection and lxc_connection plugins.

@ansibullbot

This comment was marked as outdated.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added WIP Work in progress ci_verified Push fixes to PR branch to re-run CI connection connection plugin feature This issue/PR relates to a feature request new_contributor Help guide this first time contributor plugins plugin (any type) and removed ci_verified Push fixes to PR branch to re-run CI labels Jan 31, 2025
@ansibullbot
Copy link
Collaborator

@yeetypete this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

@ansibullbot ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 31, 2025
@yeetypete yeetypete marked this pull request as ready for review January 31, 2025 19:50
@ansibullbot ansibullbot removed WIP Work in progress merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 31, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Jan 31, 2025
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Please add a changelog fragment. I've also added some first comments below.

plugins/connection/lxd.py Show resolved Hide resolved
plugins/connection/lxd.py Show resolved Hide resolved
plugins/connection/lxd.py Show resolved Hide resolved
yeetypete and others added 4 commits January 31, 2025 22:21
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
Co-authored-by: Felix Fontein <felix@fontein.de>
@yeetypete
Copy link
Author

@felixfontein will do, thanks for the comments!

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

@yeetypete
Copy link
Author

yeetypete commented Feb 2, 2025

@felixfontein @russoz One issue came up during my testing which I have pushed a fix for:
If the lxd nonroot user and local user do not have the same uid and gid, pushing temp files to the remote causes permissions issues because they are copied with the wrong uid and gid. This came up when I was testing on a GitHub runner where the uid and gid of the local user is 1001 and the lxd nonroot user was 1000. This caused gather_facts to keep failing.

I have preserved the old behavior when root is the lxd user. I guess this issue was not noticable before when the root user was always used.

@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

plugins/connection/lxd.py:165:5: E303: too many blank lines (2)
plugins/connection/lxd.py:181:5: E303: too many blank lines (2)
plugins/connection/lxd.py:196:11: E111: indentation is not a multiple of 4

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

plugins/connection/lxd.py:165:5: E303: too many blank lines (2)
plugins/connection/lxd.py:181:5: E303: too many blank lines (2)
plugins/connection/lxd.py:196:11: E111: indentation is not a multiple of 4

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

plugins/connection/lxd.py:165:5: E303: too many blank lines (2)
plugins/connection/lxd.py:181:5: E303: too many blank lines (2)
plugins/connection/lxd.py:196:11: E111: indentation is not a multiple of 4

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

plugins/connection/lxd.py:165:5: E303: too many blank lines (2)
plugins/connection/lxd.py:181:5: E303: too many blank lines (2)
plugins/connection/lxd.py:196:11: E111: indentation is not a multiple of 4

click here for bot help

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 2, 2025
@ansibullbot ansibullbot removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch check-before-release PR will be looked at again shortly before release and merged if possible. connection connection plugin feature This issue/PR relates to a feature request new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants