On 16/04/15 13:53, Martin Babinsky wrote:
On 04/16/2015 01:34 PM, Martin Babinsky wrote:
On 04/15/2015 04:17 PM, Martin Basti wrote:
https://fedorahosted.org/freeipa/ticket/4981

These patches keep usage of IPA server address as NTP server in NTP
configuration on clients, in case that no NTP servers were specified by
user, and no NTP servers were resolved from SRV records. This will
ensure backward compatibility, as IPA does not configure NTP SRV records
for each domain automatically.

Patches attached.

Martin^2


PATCH 224 NACK
PATCH 225 ACK

Patch 224 you keep the original destination (dest="ntp_server")
for --ntp-server option, but in patch 226 the code attempts to get the
server names from options.ntpservers resulting in:

Traceback (most recent call last):
   File "/sbin/ipa-client-install", line 2932, in <module>
     sys.exit(main())
   File "/sbin/ipa-client-install", line 2913, in main
     rval = install(options, env, fstore, statestore)
   File "/sbin/ipa-client-install", line 2342, in install
     if options.ntp_servers:
AttributeError: Values instance has no attribute 'ntp_servers'

So please fix this.

Naming the destination 'ntp_servers' (plural form) seems best because we
now store multiple values.

Also, if renaming "option.ntp_server" to "option.ntp_servers", do not forget to change also these lines in "ipa-client-install":

2852         if options.ntp_server:
2853             ntp_servers = options.ntp_server

(line numbers after applying patches 224-226)

Stupid me, thank you

Updated patches attached.

--
Martin Basti

From 73a920ad890ddbce953daf1317034f21da07c529 Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Tue, 14 Apr 2015 18:56:47 +0200
Subject: [PATCH 1/2] ipa client: make --ntp-server option multivalued

There can be more ntp servers in ntp.conf

Required for ticket: https://fedorahosted.org/freeipa/ticket/4981
---
 ipa-client/ipa-install/ipa-client-install | 19 +++++++++++--------
 ipa-client/ipaclient/ntpconf.py           | 11 ++++++-----
 ipa-client/man/ipa-client-install.1       |  2 +-
 3 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index 1590a08600bbb1b2fd7f4c3338b5060156d7dc38..b9c7df1485bf60c4c073cd168c8731a8130d8ac9 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -118,7 +118,9 @@ def parse_options():
     basic_group.add_option("", "--force-join", dest="force_join",
                       action="store_true", default=False,
                       help="Force client enrollment even if already enrolled")
-    basic_group.add_option("--ntp-server", dest="ntp_server", help="ntp server to use")
+    basic_group.add_option("--ntp-server", dest="ntp_servers", action="append",
+                           help="ntp server to use. This option can be used "
+                                "multiple times")
     basic_group.add_option("-N", "--no-ntp", action="store_false",
                       help="do not configure ntp", default=True, dest="conf_ntp")
     basic_group.add_option("", "--force-ntpd", dest="force_ntpd",
@@ -2330,10 +2332,11 @@ def install(options, env, fstore, statestore):
         # We assume that NTP servers are discoverable through SRV records in the DNS
         # If that fails, we try to sync directly with IPA server, assuming it runs NTP
         root_logger.info('Synchronizing time with KDC...')
-        ntp_servers = ds.ipadns_search_srv(cli_domain, '_ntp._udp', None, break_on_first=False)
+        ntp_srv_servers = ds.ipadns_search_srv(cli_domain, '_ntp._udp',
+                                               None, break_on_first=False)
         synced_ntp = False
-        if ntp_servers:
-            for s in ntp_servers:
+        if ntp_srv_servers:
+            for s in ntp_srv_servers:
                 synced_ntp = ipaclient.ntpconf.synconce_ntp(s)
                 if synced_ntp:
                     break
@@ -2838,11 +2841,11 @@ def install(options, env, fstore, statestore):
         # disable other time&date services first
         if options.force_ntpd:
             ipaclient.ntpconf.force_ntpd(statestore)
-        if options.ntp_server:
-            ntp_server = options.ntp_server
+        if options.ntp_servers:
+            ntp_servers = options.ntp_servers
         else:
-            ntp_server = cli_server[0]
-        ipaclient.ntpconf.config_ntp(ntp_server, fstore, statestore)
+            ntp_servers = cli_server
+        ipaclient.ntpconf.config_ntp(ntp_servers, fstore, statestore)
         root_logger.info("NTP enabled")
 
     if options.conf_ssh:
diff --git a/ipa-client/ipaclient/ntpconf.py b/ipa-client/ipaclient/ntpconf.py
index 7d5c82a89b51f68362f12869a9234f5b69aa5ba9..c22fba401d33009b3b95d1418dc7c8a03328d569 100644
--- a/ipa-client/ipaclient/ntpconf.py
+++ b/ipa-client/ipaclient/ntpconf.py
@@ -41,7 +41,7 @@ restrict -6 ::1
 
 # Use public servers from the pool.ntp.org project.
 # Please consider joining the pool (http://www.pool.ntp.org/join.html).
-server $SERVER
+$SERVERS_BLOCK
 
 #broadcast 192.168.1.255 key 42		# broadcast server
 #broadcastclient			# broadcast client
@@ -84,7 +84,7 @@ SYNC_HWCLOCK=yes
 NTPDATE_OPTIONS=""
 """
 ntp_step_tickers = """# Use IPA-provided NTP server for initial time
-$SERVER
+$TICKER_SERVERS_BLOCK
 """
 def __backup_config(path, fstore = None):
     if fstore:
@@ -97,12 +97,13 @@ def __write_config(path, content):
     fd.write(content)
     fd.close()
 
-def config_ntp(server_fqdn, fstore = None, sysstore = None):
+def config_ntp(ntp_servers, fstore = None, sysstore = None):
     path_step_tickers = paths.NTP_STEP_TICKERS
     path_ntp_conf = paths.NTP_CONF
     path_ntp_sysconfig = paths.SYSCONFIG_NTPD
-    sub_dict = { }
-    sub_dict["SERVER"] = server_fqdn
+    sub_dict = {}
+    sub_dict["SERVERS_BLOCK"] = "\n".join("server %s" % s for s in ntp_servers)
+    sub_dict["TICKER_SERVERS_BLOCK"] = "\n".join(ntp_servers)
 
     nc = ipautil.template_str(ntp_conf, sub_dict)
     config_step_tickers = False
diff --git a/ipa-client/man/ipa-client-install.1 b/ipa-client/man/ipa-client-install.1
index 726a6c133132dd2e3ba2fde43d8a2ec0549bfcef..978ac38c09567c101f9ad36598bb6d286284daa1 100644
--- a/ipa-client/man/ipa-client-install.1
+++ b/ipa-client/man/ipa-client-install.1
@@ -117,7 +117,7 @@ The hostname of this machine (FQDN). If specified, the hostname will be set and
 Join the host even if it is already enrolled.
 .TP
 \fB\-\-ntp\-server\fR=\fINTP_SERVER\fR
-Configure ntpd to use this NTP server.
+Configure ntpd to use this NTP server. This option can be used multiple times.
 .TP
 \fB\-N\fR, \fB\-\-no\-ntp\fR
 Do not configure or enable NTP.
-- 
2.1.0

From 1c2e78bb22980c320751edce69543533265ffb48 Mon Sep 17 00:00:00 2001
From: Martin Basti <mba...@redhat.com>
Date: Wed, 15 Apr 2015 14:32:17 +0200
Subject: [PATCH 2/2] ipa client: use NTP servers detected from SRV

Detected NTP servers from SRV records should be used in NTP client
configuration.

https://fedorahosted.org/freeipa/ticket/4981
---
 ipa-client/ipa-install/ipa-client-install | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/ipa-client/ipa-install/ipa-client-install b/ipa-client/ipa-install/ipa-client-install
index b9c7df1485bf60c4c073cd168c8731a8130d8ac9..6cfaa1dd21131b49f99f78ed3df24602b5cf80f3 100755
--- a/ipa-client/ipa-install/ipa-client-install
+++ b/ipa-client/ipa-install/ipa-client-install
@@ -2326,6 +2326,7 @@ def install(options, env, fstore, statestore):
         # hostname if different from system hostname
         tasks.backup_and_replace_hostname(fstore, statestore, options.hostname)
 
+    ntp_srv_servers = []
     if not options.on_master and options.conf_ntp:
         # Attempt to sync time with IPA server.
         # If we're skipping NTP configuration, we also skip the time sync here.
@@ -2841,10 +2842,16 @@ def install(options, env, fstore, statestore):
         # disable other time&date services first
         if options.force_ntpd:
             ipaclient.ntpconf.force_ntpd(statestore)
-        if options.ntp_servers:
+
+        if options.ntp_server:
             ntp_servers = options.ntp_servers
+        elif ntp_srv_servers:
+            ntp_servers = ntp_srv_servers
         else:
+            root_logger.warning("No SRV records of NTP servers found. IPA "
+                                "server address will be used")
             ntp_servers = cli_server
+
         ipaclient.ntpconf.config_ntp(ntp_servers, fstore, statestore)
         root_logger.info("NTP enabled")
 
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to