On 02/15/2013 08:18 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
On 02/15/2013 04:38 PM, Rob Crittenden wrote:
Petr Viktorin wrote:
ipa-replica-conncheck ran SSH in quiet mode, probably to suppress a
message about connecting to an unknown host. This made it hard to debug
connection errors.
I didn't find a way to separate SSH output from the output of the
called
command, I decided to try an additional SSH connection before calling
conncheck. SSH is set to verbose and if it fails the errors get printed
out. Also, the host is added to a temporary known_hosts file.
The second SSH is called without the -q flag so errors/warnings are not
lost even if the command fails. The temporary known_hosts file is used
so the unknown host warning doesn't appear here.
https://fedorahosted.org/freeipa/ticket/3402
The general procedure looks good, I don't think we should hardcode the
path to ssh. ipautil.run() overrides the current environment so we
should be able to safely run just 'ssh'.
We eventually need a cross-platform way of locating system binaries.
The hardcoded path to ipa-replica-conncheck is probably ok since we
provide that binary ourselves.
rob
Changed, thanks.
Looks and works well. I just have one final question. Should remote_addr
and temp_known_hosts be passed in as args? They are basically globals
but it is obvious where they came from, so not really a NAK, just a
question.
rob
Well, it's a style issue so I only have a bunch of rule-of-thumb
handwaving to justify the decision... Here goes:
Closing over the variables in main() doesn't have drawbacks of globals:
nothing outside main() can see/modify them and it doesn't keep main()
from (indirectly) calling itself (this particular main() probably won't
need to do that, but in general it's good to avoid this kind of surprises).
Passing the variables in as arguments would only make sense if run_ssh
was a top-level function, otherwise you have to worry about the right
order/names of arguments and gain nothing.
The nested function makes it clear that this is just a quick way to keep
things DRY, not a full-featured generic SSH caller. A top-level
"run_ssh" helper shouldn't unconditionally disable StrictHostKeychecking
for example. The code is tied to the caller functionally, so I kept it
tied syntactically as well.
On the other hand, nested functions are "magic" that you need a good
reason for. I think the above is just reason enough, but I don't feel
that strongly about it. Here's a version with top-level run_ssh, push it
if it seems better to you.
--
Petr³
From ba6124f8feb7232fbb9165cf8e82f56581034565 Mon Sep 17 00:00:00 2001
From: Petr Viktorin <pvikt...@redhat.com>
Date: Wed, 13 Feb 2013 08:25:11 -0500
Subject: [PATCH] Check SSH connection in ipa-replica-conncheck
Since it is not really possible to separate SSH errors from
errors of the called program, add a SSH check before
calling replica-conncheck on the master.
The check also adds the master to a temporary known_hosts file,
so suppressing SSH's warning about unknown host is no longer
necessary. If the "real" connection fails despite the check,
any SSH errors will be included in the output.
https://fedorahosted.org/freeipa/ticket/3402
---
install/tools/ipa-replica-conncheck | 46 ++++++++++++++++++++++++++++------
1 files changed, 38 insertions(+), 8 deletions(-)
diff --git a/install/tools/ipa-replica-conncheck b/install/tools/ipa-replica-conncheck
index 29c43f60bd9e38d649db3730daacb76bd45b8786..c0717d847d0352ffb87b4cd5db6a74071528c52d 100755
--- a/install/tools/ipa-replica-conncheck
+++ b/install/tools/ipa-replica-conncheck
@@ -359,16 +359,26 @@ def main():
if returncode != 0:
raise RuntimeError("Could not get ticket for master server: %s" % stderr)
- print_info("Execute check on remote master")
+ print_info("Check SSH connection to remote master")
- stderr = ''
remote_addr = "%s@%s" % (user, options.master)
- (stdout, stderr, returncode) = ipautil.run(['/usr/bin/ssh',
- '-q', '-o StrictHostKeychecking=no',
- '-o UserKnownHostsFile=/dev/null', remote_addr,
- "/usr/sbin/ipa-replica-conncheck " + " ".join(remote_check_opts)],
- env={'KRB5_CONFIG':KRB5_CONFIG, 'KRB5CCNAME' : CCACHE_FILE},
- raiseonerr=False)
+ temp_known_hosts = tempfile.NamedTemporaryFile()
+
+ stdout, stderr, returncode = run_ssh(
+ remote_addr, temp_known_hosts.name, 'echo OK', verbose=True)
+
+ if returncode != 0:
+ print 'Could not SSH into remote host. Error output:'
+ for line in stderr.splitlines():
+ print ' %s' % line
+ raise RuntimeError('Could not SSH to remote host.')
+
+ print_info("Execute check on remote master")
+
+ stdout, stderr, returncode = run_ssh(
+ remote_addr, temp_known_hosts.name,
+ "/usr/sbin/ipa-replica-conncheck " +
+ " ".join(remote_check_opts))
print_info(stdout)
@@ -384,6 +394,26 @@ def main():
time.sleep(3600)
print_info("Connection check timeout: terminating listening program")
+
+def run_ssh(remote_addr, known_hosts_filename, command, verbose=False):
+ """Run given command on remote master over SSH
+
+ Return (stdout, stderr, returncode)
+ """
+ ssh_command = ['ssh']
+ if verbose:
+ ssh_command.append('-v')
+ ssh_command += [
+ '-o StrictHostKeychecking=no',
+ '-o UserKnownHostsFile=%s' % known_hosts_filename,
+ remote_addr, command
+ ]
+ return ipautil.run(
+ ssh_command,
+ env={'KRB5_CONFIG': KRB5_CONFIG, 'KRB5CCNAME' : CCACHE_FILE},
+ raiseonerr=False)
+
+
if __name__ == "__main__":
try:
sys.exit(main())
--
1.7.7.6
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
