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

mgr/cephadm: ceph cephadm set-user does not reflect the user change in ssh-config #45796

Merged
merged 1 commit into from Apr 13, 2022

Conversation

asm0deuz
Copy link
Contributor

@asm0deuz asm0deuz commented Apr 6, 2022

Changing the user account used by cephadm for its ssh connections is not applied to the ssh-config.

Fixes: https://tracker.ceph.com/issues/54618

Signed-off-by: Teoman ONAY tonay@redhat.com

@asm0deuz asm0deuz requested a review from a team as a code owner April 6, 2022 13:59
@github-actions github-actions bot added the pybind label Apr 6, 2022
@@ -979,6 +979,9 @@ def set_ssh_user(self, user: str) -> Tuple[int, str, str]:
return 0, "value unchanged", ""

self._validate_and_set_ssh_val('ssh_user', user, current_user)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this line supposed to set the ssh_user? this code sets the new value and then calls: self.ssh._reconfig_ssh() which does the following:

....
        if self.mgr.mode == 'root':
            self.mgr.ssh_user = self.mgr.get_store('ssh_user', default='root')
        elif self.mgr.mode == 'cephadm-package':
            self.mgr.ssh_user = 'cephadm'
....

so it seems that in order to change the ssh_user self.mgr.mode must be root

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it sets the ssh_user kv but set-user should also change the value in ssh-config:

Host *
User root <------------
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
ConnectTimeout=30

This is the changes I added. I may be missing something here...

Copy link
Contributor

@adk3798 adk3798 Apr 8, 2022

Choose a reason for hiding this comment

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

To give a bit of info on the root vs. package mode thing, way back there was an idea to allow users to just have a cephadm package installed on the host that we would use for all cephadm commands. While the code for it still exists (https://github.com/ceph/ceph/blob/master/src/pybind/mgr/cephadm/serve.py#L1328) it's never properly worked and has been effectively deprecated for 2 years or so. That beings said, it would still be easy enough to respect it here. We would just need to only change the config like this in cases where if self.mode == 'root' passes. Although, since package mode isn't used at all afaik and is mostly likely just broken currently it really wouldn't make a tangible difference and I don't particularly care if we change this to worry about it or not.

Copy link
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

LGTM after these flake8 issues are fixed. As for the root vs. package thing you can fix it if you'd like but I'm okay with it either way.

src/pybind/mgr/cephadm/module.py Outdated Show resolved Hide resolved
@adk3798
Copy link
Contributor

adk3798 commented Apr 12, 2022

jenkins test api

@adk3798
Copy link
Contributor

adk3798 commented Apr 13, 2022

http://pulpito.ceph.com/adking-2022-04-13_05:16:48-orch:cephadm-wip-adk-testing-2022-04-12-2231-distro-basic-smithi/

2 Failures

To summarize, run was passing. Will start tracking this type of lvm batch failure to see if it happens on the same hosts (which would imply a possible hardware issue) in https://tracker.ceph.com/issues/55319

@adk3798 adk3798 merged commit dc827e5 into ceph:master Apr 13, 2022
@adk3798 adk3798 mentioned this pull request Apr 17, 2022
14 tasks
@adk3798 adk3798 mentioned this pull request Apr 27, 2022
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants