[crmsh-4.6] Fix: report: use ClusterShell for ssh (bsc#1220170) #1335
Conversation
nicholasyang2022
force-pushed
the
bsc_1220170_20240222
branch
from
620777d to
4e36873
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## crmsh-4.6 #1335 +/- ##
=============================================
- Coverage 52.82% 52.82% -0.01%
=============================================
Files 79 79
Lines 24002 24002
=============================================
- Hits 12679 12678 -1
- Misses 11323 11324 +1 ☔ View full report in Codecov by Sentry. |
nicholasyang2022
force-pushed
the
bsc_1220170_20240222
branch
from
c9aa25a to
9085d07
Compare
The current ssh implementation in crm.report will use a wrong user when using root + ssh-agent + sudo. This patch change to use try to ClusterShell first, providing consistent behavior with other parts of crmsh. And fallback to interactive authentication when ClusterShell does not work.
Reproducer: 1. /etc/ssh/sshd_config "PermitRootLogin yes" on both nodes 2. root@node-1 and root@node-2 are passwordless with the forwarded ssh key. That says, those nodes don't have root passwordless with their local root ssh key. 3. crm report hangs in the remote node trying to do the root passwordless ssh back to the main node to figure out the trace_ra directories
nicholasyang2022
force-pushed
the
bsc_1220170_20240222
branch
from
9085d07 to
749f0af
Compare
| @@ -441,6 +432,31 @@ def load_context_attributes(context: Context) -> None: | |||
| context.cores_dir_list.extend([constants.COROSYNC_LIB] if os.path.isdir(constants.COROSYNC_LIB) else []) | |||
|
|
|||
|
|
|||
| def load_context_trace_dir_list(context: Context) -> None: | |||
| log_contents = "" | |||
There was a problem hiding this comment.
Better to move these two comment lines (https://github.com/ClusterLabs/crmsh/blob/crmsh-4.6/crmsh/report/collect.py#L192-L193) into here to explain why need to do like this
crmsh/report/core.py
Outdated
| cmd, | ||
| ) | ||
| shell = crmsh.sh.LocalShell() | ||
| ret = shell.su_subprocess_run(None, cmd, tty=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE) |
There was a problem hiding this comment.
Is the same code as line 264-275?
There was a problem hiding this comment.
Extracted as a function _interactive_ssh_run_as_root.
nicholasyang2022
force-pushed
the
bsc_1220170_20240222
branch
from
749f0af to
886006c
Compare
nicholasyang2022
force-pushed
the
bsc_1220170_20240222
branch
from
886006c to
9510a00
Compare
|
|
||
|
|
||
| def _interactive_ssh_run_as_root(host: str, ssh_user: str, cmd: str): | ||
| # cmd MUST be escaped to make sure it works as an arugment of ssh command |
There was a problem hiding this comment.
What does this comment mean for "escaped", in this function scope?
There was a problem hiding this comment.
The cmd is concatenated to ssh as string. This string is firstly parsed by bash and later by ssh. This requires special characters in it to be escaped 2 times. And I am not sure how to write a general function to escape arguments for ssh. So for now, let the callers to escape it case by case.
The current ssh implementation in crm.report will use a wrong user when using root + ssh-agent + sudo.
This patch change to use try to ClusterShell first, providing consistent behavior with other parts of crmsh. And fallback to interactive authentication when ClusterShell does not work.