-
Notifications
You must be signed in to change notification settings - Fork 22
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
lxc_ssh uses local unix username instead of ansible_user when connecting to host #37
Comments
After digging a little, this seems caused by Line 752 in d421553
Which resets the previous (correct) value of self.user. this line was added in #31 and contradicts Line 517 in d421553
Commenting that line fixes the problem for me, but I don't have enough understanding of how ansible works to know what is the correct fix. Hopefully someone with more knowledge can tell me what's going on here |
@dmp1ce @stefangweichinger The PR #31 came from you. Can you have a look if this is an oversight, or breaks something? Thanks! |
@boucman Are you sure we are getting the full picture from your setup? When I change
Either way, a test should be created to verify the plugin is working properly, IMHO. |
From my understanding line 517 gets the Line 517 in d421553
Here is the documentation which mirrors the code in ssh.py from the official Ansible connection plugin library. Lines 254 to 269 in d421553
|
If this is something which can be replicated and the plugin is at fault, I'm happy to create an additional test. |
Can confirm this issue.
i tried every possible combination from where the user variable can be taken. but it still tries to connect using the current linux username
|
I'm just guessing here but is Maybe try again with What is the |
No environment variables are set since most of my playbooks are called from ansible_runner python api. I'm fairly new to ansible to there might be some mistakes here (sorry about that) here's my inventory file ---
all:
hosts:
type3:
ansible_host: IP.IP.IP.IP
ansible_port: 22
ansible_ssh_user: root
remote_user: root
ansible_user: root
ansible_ssh_private_key_file : /home/testuser/playbook_keys
lxc_host: 'playbook2'
ansible_connection: lxc_ssh
type2:
ansible_host: IP.IP.IP.IP
ansible_port: 22
ansible_ssh_user: root
remote_user: root
ansible_user: root
ansible_ssh_private_key_file : /home/testuser/playbook_keys
ansible_connection: ssh commenting the line like @boucman said does sorta fix the problem at hand. but now i get
looks like this is due to it not using the key mentioned in
here's the full debug log for
Logs after commenting line
|
Do you have any |
No just the ones i've written above. Also its a brand new test environment with latest ansible installed. Updated the above comment to add more logs and observations. |
There is probably an issue with the plugin but I cannot find it because I cannot reproduce the issue. Do you have steps to reproduce? Should I setup DebOps and try to reproduce that way? |
No a simple ansible playbook that pings the container using the latest ansible should do it here's the playbook ---
- name: Ping
hosts: type2
tasks:
- name: Ping Server
ping: It should not be related to DebOps as i'm not using it. using this playbook with the inventory above with command here's some more info
|
I tested your small ping-playbook with my test setup and it does NOT fail. In my case the LXC-server (the variable I have to add that all my hosts in this setup already have been bootstrapped with DebOps: that installs python stuff, sudo rights, ssh-keys etc etc. Without that bootstrap step the container doesn't have python and is not reachable via ansible. Also that additional container pings OK after installing python into it. So I also couldn't yet reproduce the issue. @AnvithLobo mentions |
it fails even before it reaches lxc part Also tried it with py 3.6 same issues can someone provide me their inventory file so i can compare it with mine ? also it it looks like the |
@AnvithLobo my inventory is in debops/debops#1731 (comment) (the related issue in the debops repo) |
yeah but i don't see a The problem with me is username on all my LXC servers are different same for the SSH keys too. Similar to OP where his local username is different than remote username The |
more debugging adding both
in So its definitely ignoring the vars set in |
As my project dir is a debops-project there's a Interesting that "None" is shown here:
Adding |
yeah it tries to use the local username / config.cfg username and not the one set in inventory to connect to the remote server. if you do not have |
Adding
leads to the playbook failing. So it's read here. |
what command are you using to run the playbook? also whats the error
then yes its being read if not then its probably not. |
I use both debops and ansible. Both read the default inventory in
debops-docs tell me: https://docs.debops.org/en/stable-2.2/introduction/getting-started.html#ansible-user I googled the difference between
|
Yes, that's the message. |
does your ansible.cfg (or generated config.cfg from debops) contain the the var |
No. |
could you run it without debops with just I tried several different machines with several different version of python and ansible without setting these in |
I did that above already. And it also fails with "breakit@172.32.99.110: Permission denied (publickey)." |
here's a log with
|
my local test setup also works OK: running my debops-setup against a Debian11-LXC-host-VM with several LXC-containers inside. But I don't have the special case with the separate |
Given all the different open PRs and comments: may I suggest a brief call and see what is required, in which order, what's still open and what can be merged? |
I agree! ;-) Quick look: #41 could be dropped now, IMO. EDIT: #42 also takes care of the removed version check ... I think it should be reviewed if we keep the version check in as suggested in faa7f94 |
#40 adds tests which actually test this plugin. I highly recommend looking over that PR and merging it before changing anything else. |
#40 is failing in the current state, but the overall approach looks good. Once the self-contained tests are working I can merge that PR. |
@stefangweichinger agreed to have a call about all of the open PRs. I'm in CEST timezone, but can accommodate most times of the day. |
Oh, you really mean a literal call, I understood your suggestion like "we should all discuss and decide now" ... like a "call to action" ;-) Does #38 now solve the original issue for @boucman and @AnvithLobo ? I am a bit biased: I would drop #41, merge #38, somehow decide what to take out of #39 and #42 (maybe only focus on #42) ... Maybe we should do it in steps: work on all this in a separate development branch until all has been resolved. |
@stefangweichinger Yes. We have 3 Issues and 6 PRs going on, and everyone is discussing everywhere, making suggestions which PR can be dropped and merged in other PRs. At least for me this is confusing, sorry. I'd like to solve this situation. We can continue discussing this in more comments. |
@andreasscherbaum I agree, it's confusing. And I agree we should get on with it. I don't know yet how to see #40: is it OK and the tested script is not, or is the test not OK yet? Can't tell right now. IMO @dmp1ce and @AnvithLobo could review and improve the PRs #40, #41, #42 and maybe distill it into one working PR. This PR might then be merged and #38 rebased on top of that. What do you think? |
If you prefer a call, why not? I assume we both speak german ;-) / I think of suggesting a PR containing all the stuff ... |
@andreasscherbaum forgive me if this confuses even more ;-) I did some ugly cherry-picking into a separate branch https://github.com/stefangweichinger/ansible-lxc-ssh/commits/alltogether Basically it contains #38, #39, #40, #42 and skips the parts where the version-checks were removed (yes, LXCv1 might not be relevant anymore ... otherwise: why not keep it in there if it doesn't do harm ... and the new way of doing the checks prepares for other future checks). If I haven't missed or screwed up something, this might be approximately what we want. The tests succeeded here: https://github.com/stefangweichinger/ansible-lxc-ssh/actions/runs/961476719 I do not yet file another PR for that .... please have a look, folks |
it does look like its working perfectly. |
I don't understand yet. @AnvithLobo : you point at a test that exists. What do you mean? |
yeah no we added a nested lxc container we need the same checks in the first container to check for LXD version like it does for the host machine. Right now it assumes that the first level of container has lxd v4. |
I think I understand but I am not sure if I can add that. Maybe you are faster than me here? ;-) |
I am trying to come up with some patches, but I lose track here: why do we need to install (and check the version of) LXC/LXD inside of Trying to store the version at first via a new |
lxd init preseed is a bit different for V3 and V4. |
#40 is a good test to the best I can tell. I copied work from @AnvithLobo mostly. The #40 PR is causing a FAIL, which is actually a good sign, because the |
I would suggest merge #40 and then new PRs can be more easily verified by checking to see if the tests pass. |
2 days later and we are still discussing in comments what is good and should be merged ;-) |
The current tests which are running are worse than nothing, because they are giving false positives. Well, they gave a good starting point to build from, but the results they are returning aren't useful. |
I understand this is complex, and quite a few people have stakes here, and want to get this improved and PRs merged. |
I'm not going to hold a hard stance on anything. I'm only trying to contribute code and keep the plugin working for people. I'm happy to discuss over chat, but I doubt I'll be able to schedule a real-time chat. |
I can confirm that the PR solves this issue, which by the way also applies to the SSH port ( |
Phew, even more PRs now, quite some activity ;-) Maybe this repo needs one or more devel-branches, in which we can do merges and develop things (people would need to create PRs against Right now it looks confusing, at least to me. The last PRs might or might not be already fixed by other older open PRs, right? |
It is confusing, indeed. |
the I'm not sure if lxc_ssh also carries over this problem and if above PR solves it. I currently stick to a workflow where paramiko_ssh sets up ssh key on container host machine and sets up most host side of things and then have lxc_ssh use that ssh key for working on containers. |
Well, aren't we running in circles here? I tried to come up with a combined step in #37 (comment) ... #42 is in there. And I am not sure if "the results they are returning aren't useful" is still true für #38 and #42. If yes, why "I would suggest merge #40 and then new PRs can be more easily verified by checking to see if the tests pass". If the tests aren't functional, why merge them and go on based on false positives? We need to agree on the next steps and their order. Discussion is good, but what is the next problem we can actually fix to proceed? |
Hello everybody
this is an upstream version of debops/debops#1731
The problem was reproduced with raw "ansible" command so we are pretty sure this is not a debops problem.
I will copy/paste the other bug report here, but some discussion have already taken place on the other side.
Hello
I am in the process of upgrading from 2.2.1 to 2.2.2
Most of my machines are LXC containers which I reach through lxc_ssh
When I attempt any playbook on build-yocto, I get a permisssion denied.
Interesting extract of my trace below:
We see that the proble is that lxc_ssh attempts to connect to the host (monstro) using my local unix username (jerros) instead of using root (I can connect directly as root on that machine, I tested separately)
This used to work in 2.2.1 and reverting to debops 2.2.1 solves the problem
Also note the line
<monstro.daviel> ESTABLISH SSH CONNECTION FOR USER: root
so debops thinks it is attempting to connect as root, but seems to be connecting without a username, so using the default, local, username.as a complement, a cutdown output of ansible-inventory for build-yocto
Thx a lot
Jérémy
The text was updated successfully, but these errors were encountered: