Moving prompt for NTP options to install_check

In a interactive installation of freeipa server a promt asks for NTP related
options after install_check has been called. As it may cause confusion to users
moving to install_check methods where the prompt for other options is being done.
Refactored sync_time() method to use passed parameters ntp_servers and ntp_pool.

Resolves: https://pagure.io/freeipa/issue/7930
Reviewed-By: Rob Crittenden <rcritten@redhat.com>
Reviewed-By: Oleg Kozlov <okozlov@redhat.com>

ipaclient/install/client.py

@@ -2085,10 +2085,13 @@ def install_check(options):
         try:
             timeconf.check_timedate_services()
         except timeconf.NTPConflictingService as e:
-            print("WARNING: conflicting time&date synchronization service '{}'"
+            print(
-                  " will be disabled".format(e.conflicting_service))
+                "WARNING: conflicting time&date synchronization service "
-            print("in favor of chronyd")
+                "'{}' will be disabled in favor of chronyd\n".format(
-            print("")
+                    e.conflicting_service
+                )
+            )
+
         except timeconf.NTPConfigurationError:
             pass
 
@@ -2374,6 +2377,11 @@ def install_check(options):
                 "Proceed with fixed values and no DNS discovery?", False):
             raise ScriptError(rval=CLIENT_INSTALL_ERROR)
 
+    if options.conf_ntp:
+        if not options.on_master and not options.unattended and not (
+                options.ntp_servers or options.ntp_pool):
+            options.ntp_servers, options.ntp_pool = timeconf.get_time_source()
+
     cli_realm = ds.realm
     cli_realm_source = ds.realm_source
     logger.debug("will use discovered realm: %s", cli_realm)
@@ -2401,6 +2409,14 @@ def install_check(options):
     logger.info("BaseDN: %s", cli_basedn)
     logger.debug("BaseDN source: %s", cli_basedn_source)
 
+    if not options.on_master:
+        if options.ntp_servers:
+            for server in options.ntp_servers:
+                logger.info("NTP server: %s", server)
+
+        if options.ntp_pool:
+            logger.info("NTP pool: %s", options.ntp_pool)
+
     # ipa-join would fail with IP address instead of a FQDN
     for srv in cli_server:
         try:
@@ -2466,7 +2482,7 @@ def update_ipa_nssdb():
                                    (nickname, sys_db.secdir, e))
 
 
-def sync_time(options, fstore, statestore):
+def sync_time(ntp_servers, ntp_pool, fstore, statestore):
     """
     Will disable any other time synchronization service and configure chrony
     with given ntp(chrony) server and/or pool using Augeas.
@@ -2478,21 +2494,24 @@ def sync_time(options, fstore, statestore):
     # disable other time&date services first
     timeconf.force_chrony(statestore)
 
-    if not options.ntp_servers and not options.ntp_pool:
+    if not ntp_servers and not ntp_pool:
+        # autodiscovery happens in case that NTP configuration isn't explicitly
+        # disabled and user did not provide any NTP server addresses or
+        # NTP pool address to the installer interactively or as an cli argument
         ds = discovery.IPADiscovery()
-        ntp_servers = ds.ipadns_search_srv(cli_domain, '_ntp._udp',
+        ntp_servers = ds.ipadns_search_srv(
-                                           None, break_on_first=False)
+            cli_domain, '_ntp._udp', None, break_on_first=False
-        if not ntp_servers and not options.unattended:
+        )
-            options.ntp_servers, options.ntp_pool = timeconf.get_time_source()
+        if ntp_servers:
-        else:
+            for server in ntp_servers:
-            options.ntp_servers = ntp_servers
+                # when autodiscovery found server records
+                logger.debug("Found DNS record for NTP server: \t%s", server)
 
     logger.info('Synchronizing time')
 
     configured = False
-    if options.ntp_servers or options.ntp_pool:
+    if ntp_servers or ntp_pool:
-        configured = timeconf.configure_chrony(options.ntp_servers,
+        configured = timeconf.configure_chrony(ntp_servers, ntp_pool,
-                                               options.ntp_pool,
                                                fstore, statestore)
     else:
         logger.warning("No SRV records of NTP servers found and no NTP server "
@@ -2577,7 +2596,7 @@ def _install(options):
 
     if options.conf_ntp:
         # Attempt to configure and sync time with NTP server (chrony).
-        sync_time(options, fstore, statestore)
+        sync_time(options.ntp_servers, options.ntp_pool, fstore, statestore)
     elif options.on_master:
         # If we're on master skipping the time sync here because it was done
         # in ipa-server-install

ipaclient/install/timeconf.py

@@ -65,7 +65,7 @@ def get_time_source():
         ntp_pool = user_input("Enter a NTP source pool address, "
                               "or press Enter to skip", allow_empty=True)
         if ntp_pool:  # if user input is not '' (empty)
-            logger.debug("User provided NTP pool:\n\t%s", ntp_pool)
+            logger.debug("User provided NTP pool:\t%s", ntp_pool)
 
     return ntp_servers, ntp_pool
 

ipaserver/install/server/install.py

@@ -16,7 +16,9 @@ import textwrap
 
 import six
 
-from ipaclient.install.client import check_ldap_conf
+from ipaclient.install import timeconf
+from ipaclient.install.client import (
+    check_ldap_conf, sync_time, restore_time_sync)
 from ipaclient.install.ipachangeconf import IPAChangeConf
 from ipalib.install import certmonger, sysrestore
 from ipapython import ipautil, version
@@ -33,7 +35,6 @@ from ipalib.util import (
     validate_domain_name,
     no_matching_interface_for_ip_address_warning,
 )
-import ipaclient.install.timeconf
 from ipaserver.install import (
     adtrust, bindinstance, ca, dns, dsinstance,
     httpinstance, installutils, kra, krbinstance,
@@ -427,13 +428,15 @@ def install_check(installer):
 
     if not options.no_ntp:
         try:
-            ipaclient.install.timeconf.check_timedate_services()
+            timeconf.check_timedate_services()
-        except ipaclient.install.timeconf.NTPConflictingService as e:
+        except timeconf.NTPConflictingService as e:
-            print("WARNING: conflicting time&date synchronization service '{}'"
+            print(
-                  " will be disabled".format(e.conflicting_service))
+                "WARNING: conflicting time&date synchronization service "
-            print("in favor of chronyd")
+                "'{}' will be disabled in favor of chronyd\n".format(
-            print("")
+                    e.conflicting_service
-        except ipaclient.install.timeconf.NTPConfigurationError:
+                )
+            )
+        except timeconf.NTPConfigurationError:
             pass
 
     if not options.setup_dns and installer.interactive:
@@ -672,6 +675,10 @@ def install_check(installer):
     if options.ip_addresses or options.setup_dns:
         installer._update_hosts_file = True
 
+    if not options.no_ntp and not options.unattended and not (
+            options.ntp_servers or options.ntp_pool):
+        options.ntp_servers, options.ntp_pool = timeconf.get_time_source()
+
     print()
     print("The IPA Master Server will be configured with:")
     print("Hostname:       %s" % host_name)
@@ -709,6 +716,14 @@ def install_check(installer):
                   "Directory unless\nthe realm name of the IPA server matches "
                   "its domain name.\n\n")
 
+    if options.ntp_servers or options.ntp_pool:
+        if options.ntp_servers:
+            for server in options.ntp_servers:
+                print("NTP server:\t{}".format(server))
+
+        if options.ntp_pool:
+            print("NTP pool:\t{}".format(options.ntp_pool))
+
     if installer.interactive and not user_input(
             "Continue to configure the system with these values?", False):
         raise ScriptError("Installation aborted")
@@ -781,11 +796,11 @@ def install(installer):
         # As chrony configuration is moved from client here, unconfiguration of
         # chrony will be handled here in uninstall() method as well by invoking
         # the ipa-server-install --uninstall
-        if not options.no_ntp:
+        if not options.no_ntp and not sync_time(
-            if not ipaclient.install.client.sync_time(options, fstore, sstore):
+                options.ntp_servers, options.ntp_pool, fstore, sstore):
-                print("Warning: IPA was unable to sync time with chrony!")
+            print("Warning: IPA was unable to sync time with chrony!")
-                print("         Time synchronization is required for IPA "
+            print("         Time synchronization is required for IPA "
-                      "to work correctly")
+                  "to work correctly")
 
         if options.dirsrv_cert_files:
             ds = dsinstance.DsInstance(fstore=fstore,
@@ -1112,7 +1127,7 @@ def uninstall(installer):
         except Exception:
             pass
 
-    ipaclient.install.client.restore_time_sync(sstore, fstore)
+    restore_time_sync(sstore, fstore)
 
     kra.uninstall()
 
@@ -1144,7 +1159,7 @@ def uninstall(installer):
 
     sstore._load()
 
-    ipaclient.install.timeconf.restore_forced_timeservices(sstore)
+    timeconf.restore_forced_timeservices(sstore)
 
     # Clean up group_exists (unused since IPA 2.2, not being set since 4.1)
     sstore.restore_state("install", "group_exists")