Merge lp:~jseutter/landscape-client/autodiscover-config into lp:~landscape/landscape-client/trunk

[Jerry Seutter]

This branch exposes config file settings for server autodiscovery. The autodiscover_srv_query_string is used when querying the DNS server with a SRV query. The autodiscover_a_query_string is used when querying the DNS server with an A query.

[lp:~jseutter/landscape-client/autodiscover-config]

455. By Jerry Seutter on 2012-02-06

query strings were being passed incorrectly for client pings.

456. By Jerry Seutter on 2012-02-06

Fixed a bug where autodiscovery was unsuccessful for message exchanges.

457. By Jerry Seutter on 2012-02-06

Reverting changes to landscape-client.conf

[Free Ekanayaka (free.ekanayaka)]

Looks good, only a couple of points about style. +1!

[1]

+def discover_server(resolver=None, autodiscover_srv_query_string="",
+ autodiscover_a_query_string=""):

and

+ lookup_deferred = discover_server(
+ None, self._autodiscover_srv_query_string,
+ self._autodiscover_a_query_string)

It feels a bit weird to pass None as first argument, the casual reader is probably going to wonder what that means, especially considering that this is the only use of discover_server() in non-test code.

If the resolver argument is used for testing only, please consider moving it at the end of the arguments list:

def discover_server(autodiscover_srv_query_string="",
                    autodiscover_a_query_string="", resolver=None):

and use it like:

+ lookup_deferred = discover_server(
+ self._autodiscover_srv_query_string,
+ self._autodiscover_a_query_string)

[2]

Not really related to this branch, but while I was reading some context, I've noticed that the ping method has grown a bit:

    def ping(self):
        """Ask the question.

        Hits the URL previously specified with the insecure_id gotten
        from the identity.

        @return: A deferred resulting in True if there are messages
            and False otherwise.
        """
        def handle_result(result):
            if result is None:
                logging.warning("Autodiscovery failed. Reverting to previous "
                                "settings.")
            else:
                result = "http://%s/ping" % result
            return result

        def do_rest(result):
            if result is not None:
                self.url = result

            insecure_id = self._identity.insecure_id
            if insecure_id is not None:
                headers = {"Content-Type": "application/x-www-form-urlencoded"}
                data = urllib.urlencode({"insecure_id": insecure_id})
                page_deferred = defer.Deferred()

                def errback(type, value, tb):
                    page_deferred.errback(Failure(value, type, tb))
                self._reactor.call_in_thread(page_deferred.callback, errback,
                                             self.get_page, self.url,
                                             post=True, data=data,
                                             headers=headers)
                page_deferred.addCallback(self._got_result)
                return page_deferred
            return defer.succeed(False)

        if self._server_autodiscover:
            lookup_deferred = discover_server(
                None, self._autodiscover_srv_query_string,
                self._autodiscover_a_query_string)
            lookup_deferred.addCallback(handle_result)
            lookup_deferred.addCallback(do_rest)
            return lookup_deferred
        else:
            return do_rest(None)

What do you think of splitting it in methods like:

    def ping(self):
        """Ask the question.

        Hits the URL previously specified with the insecure_id gotten
        from the identity.

        @return: A deferred resulting in True if there a...

review: Approve

[Chad Smith (chad.smith)]

+1 looks good tests seem to exercise the added config params.

[1] This might be a pain to set up: In transport.py this section of code doesn't seem to have a test case associated with it. I tried mangling the code to throw errors but it doesn't seem to be covered by a test. It'd be nice if we can mock the discover_server call and test _url result or warning messages logged.
        if self._server_autodiscover:
            result = blockingCallFromThread(self._reactor, discover_server,
                                            autodiscover_srv_query_string,
                                            autodiscover_a_query_string)
            if result is not None:
                self._url = "https://%s/message-system" % result
            else:
                logging.warn("Autodiscovery failed. Falling back to previous "
                             "settings.")

[2] Maybe a separate MP or maybe we already decided this to be something done during boot. Correct me if this is the wrong thought.

Upon successful auto-discover during PingClient connect, should we write those discovered values to the existing client config file so we don't autodiscover every ping attempt?

        def handle_result(result):
            if result is None:
                logging.warning("Autodiscovery failed. Reverting to previous "
                                "settings.")
            else:
                result = "http://%s/ping" % result
+? self.autodiscover = False
+? self.url = result
+? configuration.ping_url = self._url
            return result

review: Approve

=== modified file 'landscape-client.conf'
--- landscape-client.conf	2012-01-21 00:07:53 +0000
+++ landscape-client.conf	2012-02-06 22:25:22 +0000
@@ -9,5 +9,3 @@
 log_level = debug
 pid_file = /tmp/landscape/landscape-client.pid
 ping_url = http://localhost:8081/ping
-server_autodiscover = false      
-

=== modified file 'landscape/broker/config.py'
--- landscape/broker/config.py	2012-02-02 02:49:13 +0000
+++ landscape/broker/config.py	2012-02-06 22:25:22 +0000
@@ -45,6 +45,9 @@
               - C{otp}
               - C{record}
               - C{provisioning_otp}
+              - C{server_autodiscover}
+              - C{autodiscover_srv_query_string}
+              - C{autodiscover_a_query_string}
         """
         parser = super(BrokerConfiguration, self).make_parser()
 
@@ -91,6 +94,13 @@
                           help="The OTP to use for a provisioned machine.")
         parser.add_option("--server-autodiscover", type="string",
                           default="false", help="Enable server autodiscovery.")
+        parser.add_option("--autodiscover-srv-query-string", type="string",
+                          default="_tcp._landscape.localdomain",
+                          help="autodiscovery string for DNS SRV queries")
+        parser.add_option("--autodiscover-a-query-string", type="string",
+                          default="landscape.localdomain",
+                          help="autodiscovery string for DNS A queries")
+
         return parser
 
     @property

=== modified file 'landscape/broker/dnslookup.py'
--- landscape/broker/dnslookup.py	2012-02-02 01:39:15 +0000
+++ landscape/broker/dnslookup.py	2012-02-06 22:25:22 +0000
@@ -5,31 +5,36 @@
 from twisted.names.client import Resolver
 
 
-def discover_server(resolver=None):
+def discover_server(resolver=None, autodiscover_srv_query_string="",
+                    autodiscover_a_query_string=""):
     """
     Look up the dns location of the landscape server.
 
     @type resolver: The resolver to use.  If none is specified a resolver that
         uses settings from /etc/resolv.conf will be created.
+    @param autodiscover_srv_query_string: The query string to send to the DNS
+        server when making a SRV query.
+    @param autodiscover_a_query_string: The query string to send to the DNS
+        server when making a A query.
     """
     if not resolver:
         resolver = Resolver("/etc/resolv.conf")
-    d = lookup_server_record(resolver)
-    d.addErrback(lookup_hostname, resolver)
+    d = lookup_server_record(resolver, autodiscover_srv_query_string)
+    d.addErrback(lookup_hostname, resolver, autodiscover_a_query_string)
     return d
 
 
-def lookup_server_record(resolver):
+def lookup_server_record(resolver, service_name):
     """
     Do a DNS SRV record lookup for the location of the landscape server.
 
     @type resolver: A resolver to use for DNS lookups
         L{twisted.names.client.Resolver}.
+    @param service_name: The query string to send to the DNS server when
+        making a SRV query.
     @return: A deferred containing either the hostname of the landscape server
         if found or an empty string if not found.
     """
-    service_name = "_landscape._tcp.mylandscapehost.com"
-
     def lookup_done(result):
         name = ""
         for item in result:
@@ -49,17 +54,17 @@
     return d
 
 
-def lookup_hostname(result, resolver):
+def lookup_hostname(result, resolver, hostname):
     """
     Do a DNS name lookup for the location of the landscape server.
 
     @param result: The result from a call to lookup_server_record.
     @param resolver: The resolver to use for DNS lookups.
+    @param hostname: The query string to send to the DNS server when making
+        a A query.
     @param return: A deferred containing the ip address of the landscape
         server if found or None if not found.
     """
-    hostname = "landscape.localdomain"
-
     def lookup_done(result):
         return result
 

=== modified file 'landscape/broker/ping.py'
--- landscape/broker/ping.py	2012-01-27 22:00:02 +0000
+++ landscape/broker/ping.py	2012-02-06 22:25:22 +0000
@@ -22,10 +22,19 @@
     @param url: The URL to ask the question to.
     @type identity: L{landscape.broker.registration.Identity}
     @param identity: This client's identity.
+    @param get_page: The method to use to retrieve content.  If not specified,
+        landscape.lib.fetch.fetch is used.
+    @param server_autodiscovery: Server autodiscovery is performed if True,
+        otherwise server autodiscover is not done.
+    @param autodiscover_srv_query_string: If server autodiscovery is done, this
+        string will be sent to the DNS server when making a SRV query.
+    @param autodiscover_a_query_string: If server autodiscovery is done, this
+        string will be sent to the DNS server when making an A query.
     """
 
     def __init__(self, reactor, url, identity, get_page=None,
-                 server_autodiscover=False):
+                 server_autodiscover=False, autodiscover_srv_query_string="",
+                 autodiscover_a_query_string=""):
         if get_page is None:
             get_page = fetch
         self._reactor = reactor
@@ -33,6 +42,8 @@
         self.get_page = get_page
         self.url = url
         self._server_autodiscover = server_autodiscover
+        self._autodiscover_srv_query_string = autodiscover_srv_query_string
+        self._autodiscover_a_query_string = autodiscover_a_query_string
 
     def ping(self):
         """Ask the question.
@@ -72,7 +83,9 @@
             return defer.succeed(False)
 
         if self._server_autodiscover:
-            lookup_deferred = discover_server()
+            lookup_deferred = discover_server(
+                None, self._autodiscover_srv_query_string,
+                self._autodiscover_a_query_string)
             lookup_deferred.addCallback(handle_result)
             lookup_deferred.addCallback(do_rest)
             return lookup_deferred
@@ -103,7 +116,8 @@
 
     def __init__(self, reactor, url, identity, exchanger,
                  interval=30, ping_client_factory=PingClient,
-                 server_autodiscover=False):
+                 server_autodiscover=False, autodiscover_srv_query_string="",
+                 autodiscover_a_query_string=""):
         self._url = url
         self._interval = interval
         self._identity = identity
@@ -113,6 +127,8 @@
         self._ping_client = None
         self.ping_client_factory = ping_client_factory
         self._server_autodiscover = server_autodiscover
+        self._autodiscover_srv_query_string = autodiscover_srv_query_string
+        self._autodiscover_a_query_string = autodiscover_a_query_string
         reactor.call_on("message", self._handle_set_intervals)
 
     def get_url(self):
@@ -130,7 +146,9 @@
         """Start pinging."""
         self._ping_client = self.ping_client_factory(
             self._reactor, self._url, self._identity,
-            server_autodiscover=self._server_autodiscover)
+            server_autodiscover=self._server_autodiscover,
+            autodiscover_srv_query_string=self._autodiscover_srv_query_string,
+            autodiscover_a_query_string=self._autodiscover_a_query_string)
         self._call_id = self._reactor.call_every(self._interval, self.ping)
 
     def ping(self):

=== modified file 'landscape/broker/service.py'
--- landscape/broker/service.py	2012-01-30 16:11:20 +0000
+++ landscape/broker/service.py	2012-02-06 22:25:22 +0000
@@ -55,7 +55,9 @@
 
         self.transport = self.transport_factory(
             self.reactor, config.url, config.ssl_public_key,
-            self.payload_recorder, config.server_autodiscover)
+            self.payload_recorder, config.server_autodiscover,
+            config.autodiscover_srv_query_string,
+            config.autodiscover_a_query_string)
         self.message_store = get_default_message_store(
             self.persist, config.message_store_path)
         self.identity = Identity(self.config, self.persist)
@@ -66,7 +68,9 @@
         self.pinger = self.pinger_factory(
             self.reactor, config.ping_url, self.identity, self.exchanger,
             interval=config.ping_interval,
-            server_autodiscover=config.server_autodiscover)
+            server_autodiscover=config.server_autodiscover,
+            autodiscover_srv_query_string=config.autodiscover_srv_query_string,
+            autodiscover_a_query_string=config.autodiscover_a_query_string)
         self.registration = RegistrationHandler(
             config, self.identity, self.reactor, self.exchanger, self.pinger,
             self.message_store, fetch_async)

=== modified file 'landscape/broker/tests/test_config.py'
--- landscape/broker/tests/test_config.py	2012-02-02 02:49:13 +0000
+++ landscape/broker/tests/test_config.py	2012-02-06 22:25:22 +0000
@@ -92,19 +92,19 @@
         self.assertEqual(configuration.url,
                          "https://landscape.canonical.com/message-system")
 
-    def test_server_autodiscovery_handling(self):
+    def test_server_autodiscover_handling(self):
         """
         server_autodiscover is parsed and converted to a boolean value by
         load().
         """
         configuration = BrokerConfiguration()
         configuration.load([])
-        self.assertEquals(configuration.server_autodiscover, False)
+        self.assertEqual(configuration.server_autodiscover, False)
 
         configuration = BrokerConfiguration()
         configuration.load(["--server-autodiscover=true"])
-        self.assertEquals(configuration.server_autodiscover, True)
+        self.assertEqual(configuration.server_autodiscover, True)
 
         configuration = BrokerConfiguration()
         configuration.load(["--server-autodiscover=false"])
-        self.assertEquals(configuration.server_autodiscover, False)
+        self.assertEqual(configuration.server_autodiscover, False)

=== modified file 'landscape/broker/tests/test_dnslookup.py'
--- landscape/broker/tests/test_dnslookup.py	2012-02-02 01:39:15 +0000
+++ landscape/broker/tests/test_dnslookup.py	2012-02-06 22:25:22 +0000
@@ -46,13 +46,16 @@
     def __init__(self):
         self.results = None
         self.name = None
+        self.queried = None
 
     def lookupService(self, arg1):
+        self.queried = arg1
         deferred = defer.Deferred()
         deferred.callback(self.results)
         return deferred
 
     def getHostByName(self, arg1):
+        self.queried = arg1
         deferred = defer.Deferred()
         deferred.callback(self.name)
         return deferred
@@ -84,11 +87,13 @@
         fake_result.payload.target.name = "a.b.com"
         fake_resolver = FakeResolver()
         fake_resolver.results = [[fake_result]]
+        query_string = "_landscape._tcp.mylandscapehost.com"
 
         def check(result):
-            self.assertEquals("a.b.com", result)
+            self.assertEqual(fake_resolver.queried, query_string)
+            self.assertEqual("a.b.com", result)
 
-        d = lookup_server_record(fake_resolver)
+        d = lookup_server_record(fake_resolver, query_string)
         d.addCallback(check)
         return d
 
@@ -101,9 +106,10 @@
         fake_resolver.results = [[]]
 
         def check(result):
-            self.assertEquals("", result)
+            self.assertEqual("", result)
 
-        d = lookup_server_record(fake_resolver)
+        d = lookup_server_record(fake_resolver,
+                                 "_landscape._tcp.mylandscapehost.com")
         d.addCallback(check)
         return d
 
@@ -115,7 +121,8 @@
                      "failed.")
         self.mocker.replay()
 
-        d = lookup_server_record(BadResolver())
+        d = lookup_server_record(BadResolver(),
+                                 "_landscape._tcp.mylandscapehost.com")
         self.assertFailure(d, ResolverError)
         return d
 
@@ -127,11 +134,13 @@
         """
         fake_resolver = FakeResolver()
         fake_resolver.name = "a.b.com"
+        query_string = "landscape.localdomain"
 
         def check(result):
-            self.assertEquals("a.b.com", result)
+            self.assertEqual(fake_resolver.queried, query_string)
+            self.assertEqual("a.b.com", result)
 
-        d = lookup_hostname(None, fake_resolver)
+        d = lookup_hostname(None, fake_resolver, query_string)
         d.addCallback(check)
         return d
 
@@ -144,9 +153,9 @@
         fake_resolver.name = None
 
         def check(result):
-            self.assertEquals(None, result)
+            self.assertEqual(None, result)
 
-        d = lookup_hostname(None, fake_resolver)
+        d = lookup_hostname(None, fake_resolver, "landscape.localdomain")
         d.addCallback(check)
         return d
 
@@ -157,7 +166,7 @@
         logging_mock("Name lookup of landscape.localdomain failed.")
         self.mocker.replay()
 
-        d = lookup_hostname(None, BadResolver())
+        d = lookup_hostname(None, BadResolver(), "landscape.localdomain")
         self.assertFailure(d, ResolverError)
         return d
 
@@ -174,7 +183,7 @@
         d = discover_server(fake_resolver)
 
         def check(result):
-            self.assertEquals("a.b.com", result)
+            self.assertEqual("a.b.com", result)
 
         d.addCallback(check)
         return d
@@ -187,13 +196,13 @@
         d = discover_server(fake_resolver)
 
         def check(result):
-            self.assertEquals("x.y.com", result)
+            self.assertEqual("x.y.com", result)
 
         d.addCallback(check)
         return d
 
     def test_failed_lookup(self):
         """A resolver error is returned when server autodiscovery fails."""
-        d = lookup_server_record(BadResolver())
+        d = lookup_server_record(BadResolver(), "landscape.localdomain")
         self.assertFailure(d, ResolverError)
         return d

=== modified file 'landscape/broker/tests/test_ping.py'
--- landscape/broker/tests/test_ping.py	2012-01-19 23:34:55 +0000
+++ landscape/broker/tests/test_ping.py	2012-02-06 22:25:22 +0000
@@ -128,7 +128,9 @@
         self.url = "http://localhost:8081/whatever"
         self.page_getter = FakePageGetter(None)
 
-        def factory(reactor, url, insecure_id, server_autodiscover):
+        def factory(reactor, url, insecure_id, server_autodiscover,
+                    autodiscover_srv_query_string,
+                    autodiscover_a_query_string):
             return PingClient(reactor, url, insecure_id,
                               get_page=self.page_getter.get_page)
         self.pinger = Pinger(self.broker_service.reactor,

=== modified file 'landscape/broker/transport.py'
--- landscape/broker/transport.py	2012-01-26 23:37:53 +0000
+++ landscape/broker/transport.py	2012-02-06 22:25:22 +0000
@@ -22,15 +22,24 @@
     @param pubkey: SSH public key used for secure communication.
     @param payload_recorder: PayloadRecorder used for recording exchanges
         with the server.  If C{None}, exchanges will not be recorded.
+    @param server_autodiscovery: Server autodiscovery is performed if True,
+        otherwise server autodiscover is not done.
+    @param autodiscover_srv_query_string: If server autodiscovery is done, this
+        string will be sent to the DNS server when making a SRV query.
+    @param autodiscover_a_query_string: If server autodiscovery is done, this
+        string will be sent to the DNS server when making an A query.
     """
 
     def __init__(self, reactor, url, pubkey=None, payload_recorder=None,
-                 server_autodiscover=False):
+                 server_autodiscover=False, autodiscover_srv_query_string="",
+                 autodiscover_a_query_string=""):
         self._reactor = reactor
         self._url = url
         self._pubkey = pubkey
         self._payload_recorder = payload_recorder
         self._server_autodiscover = server_autodiscover
+        self._autodiscover_srv_query_string = autodiscover_srv_query_string
+        self._autodiscover_a_query_string = autodiscover_a_query_string
 
     def get_url(self):
         """Get the URL of the remote message system."""
@@ -42,7 +51,10 @@
 
     def _curl(self, payload, computer_id, message_api):
         if self._server_autodiscover:
-            result = blockingCallFromThread(self._reactor, discover_server)
+            result = blockingCallFromThread(
+                self._reactor, discover_server, None,
+                self._autodiscover_srv_query_string,
+                self._autodiscover_a_query_string)
             if result is not None:
                 self._url = "https://%s/message-system" % result
             else:
@@ -166,7 +178,9 @@
     """Fake transport for testing purposes."""
 
     def __init__(self, reactor=None, url=None, pubkey=None,
-                 payload_recorder=None, server_autodiscover=False):
+                 payload_recorder=None, server_autodiscover=False,
+                 autodiscover_srv_query_string="",
+                 autodiscover_a_query_string=""):
         self._pubkey = pubkey
         self._payload_recorder = payload_recorder
         self.payloads = []
@@ -179,6 +193,8 @@
         self._url = url
         self._reactor = reactor
         self._server_autodiscover = server_autodiscover
+        self._autodiscover_srv_query_string = autodiscover_srv_query_string
+        self._autodiscover_a_query_string = autodiscover_a_query_string
 
     def get_url(self):
         return self._url