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

[Jerry Seutter]

This branch adds server autodiscovery to landscape client.

Currently the strings used during SRV and name lookup are hardcoded. A future branch will add proper parsing and saving of these settings in the config file.

[Thomas Herve (therve)]

[1]
+ if self.server_autodiscover.lower() == "true":
+ self.server_autodiscover = True
+ else:
+ self.server_autodiscover = Fals

This is untested, and doesn't work because you didn't declare the config value.

[2] There are several test failures mostly in test_config.

[3] Having the methods being class methods on BrokerService isn't much help. What about having a free function?

[4] All functions and tests must have docstrings.

[5]
+ elif row.type == dns.A:
+ pass
+ else:
+ pass

Are those clauses necessary?

[6]
+ def lookup_failed(result):
+ logging.info("SRV lookup of %s failed." % service_name)
+ return result

This is untested.

[7]
+ def lookup_failed(result):
+ logging.info("Name lookup of %s failed." % hostname)
+ return None

And this seems to be as well.

[8]
+ def handle_result(result):
+ if result is None:
+ logging.warning("Autodiscovery failed. Reverting to previous "
+ "settings.")
+ return result
+
+ def do_rest(result):
+ if result:
+ self.url = "http://%s/ping" % result

This is slightly strange, because you encode the logic of autodiscovery returning None in both places. You should return the URL in handle_result no matter what.

[9] It'd be nice to cook an DNS server script to answer those kind of requests, so that we can test locally easily.

Thanks!

review: Needs Fixing

[Jerry Seutter (jseutter)]

Hi Thomas,

1,2,3,4,5,6,7 should be fixed.

> [8]
> + def handle_result(result):
> + if result is None:
> + logging.warning("Autodiscovery failed. Reverting to previous
> "
> + "settings.")
> + return result
> +
> + def do_rest(result):
> + if result:
> + self.url = "http://%s/ping" % result
>
> This is slightly strange, because you encode the logic of autodiscovery
> returning None in both places. You should return the URL in handle_result no
> matter what.

Yeah, the code is a bit tortured here. I tried cleaning it up but without much effect. The problem is that a deferred is created only when autodiscovery is enabled. When autodiscovery is disabled the code calls do_rest(None), so checking for None is done in both places. Were you thinking of a better way to do the same thing?

> [9] It'd be nice to cook an DNS server script to answer those kind of
> requests, so that we can test locally easily.

I added a small script to the dev/ directory that developers can run on their local system. Using it will also require editing /etc/resolv.conf manually to point the dns client to this server.

[Thomas Herve (therve)]

[10] pep8

landscape/broker/tests/test_dnslookup.py:109:9: E301 expected 1 blank line, found 0
landscape/broker/tests/test_dnslookup.py:158:9: E301 expected 1 blank line, found 0
landscape/tests/helpers.py:616:27: E261 at least two spaces before inline comment
landscape/tests/helpers.py:647:1: E302 expected 2 blank lines, found 1

[11] pyflakes

landscape/broker/service.py:4: 'logging' imported but unused
landscape/broker/service.py:5: 'dns' imported but unused
landscape/broker/service.py:6: 'Resolver' imported but unused
landscape/broker/tests/test_service.py:9: 'defer' imported but unused
landscape/broker/dnslookup.py:13: undefined name '_lookup_server_record'
landscape/broker/dnslookup.py:14: undefined name '_lookup_hostname'
dev/dns_server.py:4: 'twisted' imported but unused
dev/dns_server.py:71: local variable 'resolver' is assigned to but never us

Ooops. Please add tests to discover_server.

[12]
+ error_result = []
+ def check_error(result):
+ error_result.append(result.value)
+
+ d = lookup_hostname(None, bad_resolver)
+ d.addErrback(check_error)
+
+ self.assertIsInstance(error_result[0], ResolverError)

To properly check for errors, you should use assertFailure, and return the Deferred in your test.

[13] Can you add a docstring to dev/dns_server.py, explaining what to put in /etc/resolv.conf to use it?

Thanks, almost there!

review: Needs Fixing

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

446. By Jerry Seutter on 2012-01-30

lintian cleanups.

447. By Jerry Seutter on 2012-01-30

- Adding missing unit tests
- Adding missing docstrings
- Renaming dns_server.py to dns-server

448. By Jerry Seutter on 2012-01-30

Better documentation.

[Jerry Seutter (jseutter)]

10,11,12,13 are fixed. I added a specification on the wiki for how to configure bind 9 to answer requests and included the link in the dns-server docstring.

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

449. By Jerry Seutter on 2012-01-30

Resolver creation code was broken.

[Thomas Herve (therve)]

[14]
+ SRV_RESPONSE = args["srv_response"]
+ A_RESPONSE = args["a_response"]

This means you always need to pass the options, the defaults defined above aren't used.

[15]
+ name = row.payload.target

The type of target is a dns.Name instance, not a simple string (it works because they have a correct __str__). Can you return name.name instead, and fix the tests?

I guess what remains is the customization of the service name and the hostname? You plan to do that in the next branch?

Thanks:

review: Approve

[Frank Wierzbicki (fwierzbicki)]

Looks good +1!

review: Approve

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

450. By Jerry Seutter on 2012-02-02

- Now extract the srv name from the Name result correctly.
- Defaults on dns-server script now work correctly.

451. By Jerry Seutter on 2012-02-02

Fixed parsing of server_autodiscover=false.

452. By Jerry Seutter on 2012-02-02

Removing an unnecessary change.

[Jerry Seutter (jseutter)]

> [14]
> + SRV_RESPONSE = args["srv_response"]
> + A_RESPONSE = args["a_response"]
>
> This means you always need to pass the options, the defaults defined above
> aren't used.

You're right. This is fixed now.

> [15]
> + name = row.payload.target
>
> The type of target is a dns.Name instance, not a simple string (it works
> because they have a correct __str__). Can you return name.name instead, and
> fix the tests?

Fixed as well.

> I guess what remains is the customization of the service name and the
> hostname? You plan to do that in the next branch?

Yes, I plan to do this next in a new branch.

Since you looked at this last I found a bug with parsing the config file value. I fixed it and made the existing test more thorough.

I also removed the server_autodiscover @property from the ConfigController as I couldn't see where it was used. The code still works without it.

I don't know if it is customary to look over the changes again or not.

Thanks for your reviews!

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

453. By Jerry Seutter on 2012-02-03

Merge from trunk.

=== added file 'dev/dns-server'
--- dev/dns-server	1970-01-01 00:00:00 +0000
+++ dev/dns-server	2012-02-03 19:35:22 +0000
@@ -0,0 +1,99 @@
+#!/usr/bin/env python
+"""
+This script creates a DNS server that has enough functionality to test
+the server autodiscovery feature of landscape client.
+
+Landscape client uses /etc/resolv.conf to find the location of name servers.
+To have landscape client use this server, make sure that:
+
+nameserver 127.0.0.1
+
+is the first nameserver in /etc/resolv.conf.  Linux name lookups only support
+port 53, so this program must run on port 53 in order to work.  Be aware that
+NetworkManager overwrites this file any time your network configuration
+changes.
+
+This program does not return enough information for a tool like dig to complete
+successfully.  If this is needed, use Bind 9, detailed at
+https://wiki.canonical.com/Landscape/SpecRegistry/0082
+"""
+
+import argparse
+import sys
+from twisted.internet import reactor, defer
+from twisted.names import dns, common
+from twisted.names.server import DNSServerFactory
+
+
+PORT = 5553
+SRV_RESPONSE = 'lds1.mylandscapehost.com'
+A_RESPONSE = '127.0.0.1'
+
+
+class SimpleResolver(common.ResolverBase):
+    def _lookup(self, name, cls, typ, timeout):
+        """
+        Respond to DNS requests.  See documentation for
+        L{twisted.names.common.ResolverBase}.
+        """
+        # This nameserver returns the same result all the time, regardless
+        # of what name the client asks for.
+        results = []
+        ttl = 60
+        if typ == dns.SRV:
+            record = dns.Record_SRV(0, 1, 80, SRV_RESPONSE, ttl)
+            owner = '_landscape._tcp.mylandscapehost.com'
+            results.append(dns.RRHeader(owner, record.TYPE, dns.IN, ttl,
+                                        record, auth=True))
+        elif typ == dns.A:
+            record = dns.Record_A(A_RESPONSE)
+            owner = 'landscape.localdomain'
+            results.append(dns.RRHeader(owner, record.TYPE, dns.IN, ttl,
+                                        record, auth=True))
+
+        authority = []
+        return defer.succeed((results, authority, []))
+
+
+def parse_command_line(args):
+    global SRV_RESPONSE, A_RESPONSE, PORT
+    description = """
+    This test tool responds to DNS queries for SRV and A records.  It always
+    responds with the same result regardless of the query string sent by the
+    client.
+
+    To test this tool, try the following commands:
+    dig -p 5553 @127.0.0.1 SRV _landscape._tcp.mylandscapehost.com
+
+    dig -p 5553 @127.0.0.1 localhost.localdomain
+    """
+    parser = argparse.ArgumentParser(description=description)
+    parser.add_argument("--srv-response", type=str, default=SRV_RESPONSE,
+                        help="Give this reply to SRV queries (eg: localhost)")
+    parser.add_argument("--a-response", type=str, default=A_RESPONSE,
+                        help="Give this reply to A queries (eg: 127.0.0.1)")
+    parser.add_argument("--port", type=int, default=PORT,
+                        help="Listen on this port (default 5553).  DNS "
+                        "normally runs on port 53")
+
+    args = vars(parser.parse_args())
+    SRV_RESPONSE = args["srv_response"]
+    A_RESPONSE = args["a_response"]
+    PORT = args["port"]
+
+
+def main():
+    parse_command_line(sys.argv)
+
+    simple_resolver = SimpleResolver()
+    factory = DNSServerFactory(authorities=[simple_resolver], verbose=1)
+    protocol = dns.DNSDatagramProtocol(factory)
+    print "starting reactor on port %s.." % PORT
+    reactor.listenTCP(PORT, factory)
+    reactor.listenUDP(PORT, protocol)
+    reactor.run()
+    print "reactor stopped..."
+
+
+if __name__ == "__main__":
+    main()

=== modified file 'landscape-client.conf'
--- landscape-client.conf	2012-01-11 16:33:15 +0000
+++ landscape-client.conf	2012-02-03 19:35:22 +0000
@@ -9,3 +9,5 @@
 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-01-09 09:54:49 +0000
+++ landscape/broker/config.py	2012-02-03 19:35:22 +0000
@@ -89,6 +89,8 @@
                           help="Record data sent to the server on filesystem.")
         parser.add_option("--provisioning-otp", default="",
                           help="The OTP to use for a provisioned machine.")
+        parser.add_option("--server-autodiscover", type="string",
+                          default="false", help="Enable server autodiscovery.")
         return parser
 
     @property
@@ -118,3 +120,6 @@
 
         if self.url is None:
             self.url = self.DEFAULT_URL
+
+        autodiscover = str(self.server_autodiscover).lower()
+        self.server_autodiscover = (autodiscover == "true")

=== added file 'landscape/broker/dnslookup.py'
--- landscape/broker/dnslookup.py	1970-01-01 00:00:00 +0000
+++ landscape/broker/dnslookup.py	2012-02-03 19:35:22 +0000
@@ -0,0 +1,73 @@
+"""DNS lookups for server autodiscovery."""
+
+import logging
+from twisted.names import dns
+from twisted.names.client import Resolver
+
+
+def discover_server(resolver=None):
+    """
+    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.
+    """
+    if not resolver:
+        resolver = Resolver("/etc/resolv.conf")
+    d = lookup_server_record(resolver)
+    d.addErrback(lookup_hostname, resolver)
+    return d
+
+
+def lookup_server_record(resolver):
+    """
+    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}.
+    @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:
+            for row in item:
+                if row.type == dns.SRV:
+                    name = row.payload.target.name
+                    break
+        return name
+
+    def lookup_failed(result):
+        logging.info("SRV lookup of %s failed." % service_name)
+        return result
+
+    d = resolver.lookupService(service_name)
+    d.addCallback(lookup_done)
+    d.addErrback(lookup_failed)
+    return d
+
+
+def lookup_hostname(result, resolver):
+    """
+    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 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
+
+    def lookup_failed(result):
+        logging.info("Name lookup of %s failed." % hostname)
+        return result
+
+    d = resolver.getHostByName(hostname)
+    d.addCallback(lookup_done)
+    d.addErrback(lookup_failed)
+    return d

=== modified file 'landscape/broker/ping.py'
--- landscape/broker/ping.py	2011-07-21 23:55:47 +0000
+++ landscape/broker/ping.py	2012-02-03 19:35:22 +0000
@@ -4,6 +4,7 @@
 """
 
 import urllib
+import logging
 from logging import info
 
 from twisted.python.failure import Failure
@@ -12,6 +13,7 @@
 from landscape.lib.bpickle import loads
 from landscape.lib.fetch import fetch
 from landscape.lib.log import log_failure
+from landscape.broker.dnslookup import discover_server
 
 
 class PingClient(object):
@@ -22,13 +24,15 @@
     @param identity: This client's identity.
     """
 
-    def __init__(self, reactor, url, identity, get_page=None):
+    def __init__(self, reactor, url, identity, get_page=None,
+                 server_autodiscover=False):
         if get_page is None:
             get_page = fetch
         self._reactor = reactor
         self._identity = identity
         self.get_page = get_page
         self.url = url
+        self._server_autodiscover = server_autodiscover
 
     def ping(self):
         """Ask the question.
@@ -39,20 +43,41 @@
         @return: A deferred resulting in True if there are messages
             and False otherwise.
         """
-        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)
+        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()
+            lookup_deferred.addCallback(handle_result)
+            lookup_deferred.addCallback(do_rest)
+            return lookup_deferred
+        else:
+            return do_rest(None)
 
     def _got_result(self, webtext):
         """
@@ -77,7 +102,8 @@
     """
 
     def __init__(self, reactor, url, identity, exchanger,
-                 interval=30, ping_client_factory=PingClient):
+                 interval=30, ping_client_factory=PingClient,
+                 server_autodiscover=False):
         self._url = url
         self._interval = interval
         self._identity = identity
@@ -86,6 +112,7 @@
         self._call_id = None
         self._ping_client = None
         self.ping_client_factory = ping_client_factory
+        self._server_autodiscover = server_autodiscover
         reactor.call_on("message", self._handle_set_intervals)
 
     def get_url(self):
@@ -101,8 +128,9 @@
 
     def start(self):
         """Start pinging."""
-        self._ping_client = self.ping_client_factory(self._reactor, self._url,
-                                                     self._identity)
+        self._ping_client = self.ping_client_factory(
+            self._reactor, self._url, self._identity,
+            server_autodiscover=self._server_autodiscover)
         self._call_id = self._reactor.call_every(self._interval, self.ping)
 
     def ping(self):
@@ -119,7 +147,8 @@
 
     def _got_error(self, failure):
         log_failure(failure,
-                    "Error contacting ping server at %s" % (self._url,))
+                    "Error contacting ping server at %s" %
+                    (self._ping_client.url,))
 
     def _handle_set_intervals(self, message):
         if message["type"] == "set-intervals" and "ping" in message:

=== modified file 'landscape/broker/service.py'
--- landscape/broker/service.py	2011-08-19 15:47:27 +0000
+++ landscape/broker/service.py	2012-02-03 19:35:22 +0000
@@ -52,8 +52,10 @@
             self.payload_recorder = PayloadRecorder(config.record_directory)
         else:
             self.payload_recorder = None
+
         self.transport = self.transport_factory(
-            config.url, config.ssl_public_key, self.payload_recorder)
+            self.reactor, config.url, config.ssl_public_key,
+            self.payload_recorder, config.server_autodiscover)
         self.message_store = get_default_message_store(
             self.persist, config.message_store_path)
         self.identity = Identity(self.config, self.persist)
@@ -61,9 +63,10 @@
         self.exchanger = MessageExchange(
             self.reactor, self.message_store, self.transport, self.identity,
             exchange_store, config)
-        self.pinger = self.pinger_factory(self.reactor, config.ping_url,
-                                          self.identity, self.exchanger,
-                                          interval=config.ping_interval)
+        self.pinger = self.pinger_factory(
+            self.reactor, config.ping_url, self.identity, self.exchanger,
+            interval=config.ping_interval,
+            server_autodiscover=config.server_autodiscover)
         self.registration = RegistrationHandler(
             config, self.identity, self.reactor, self.exchanger, self.pinger,
             self.message_store, fetch_async)

=== modified file 'landscape/broker/tests/helpers.py'
--- landscape/broker/tests/helpers.py	2011-03-25 08:42:48 +0000
+++ landscape/broker/tests/helpers.py	2012-02-03 19:35:22 +0000
@@ -37,6 +37,7 @@
             "computer_title = Some Computer\n"
             "account_name = some_account\n"
             "ping_url = http://localhost:91910\n"
+            "server_autodiscover = false\n"
             "data_path = %s\n"
             "log_dir = %s\n" % (data_path, log_dir))
 
@@ -74,7 +75,7 @@
         test_case.mstore = get_default_message_store(
             test_case.persist, test_case.config.message_store_path)
         test_case.identity = Identity(test_case.config, test_case.persist)
-        test_case.transport = FakeTransport(test_case.config.url,
+        test_case.transport = FakeTransport(None, test_case.config.url,
                                             test_case.config.ssl_public_key)
         test_case.reactor = FakeReactor()
         test_case.exchange_store = ExchangeStore(

=== modified file 'landscape/broker/tests/test_config.py'
--- landscape/broker/tests/test_config.py	2012-01-09 15:14:12 +0000
+++ landscape/broker/tests/test_config.py	2012-02-03 19:35:22 +0000
@@ -91,3 +91,20 @@
 
         self.assertEqual(configuration.url,
                          "https://landscape.canonical.com/message-system")
+
+    def test_server_autodiscovery_handling(self):
+        """
+        server_autodiscover is parsed and converted to a boolean value by
+        load().
+        """
+        configuration = BrokerConfiguration()
+        configuration.load([])
+        self.assertEquals(configuration.server_autodiscover, False)
+
+        configuration = BrokerConfiguration()
+        configuration.load(["--server-autodiscover=true"])
+        self.assertEquals(configuration.server_autodiscover, True)
+
+        configuration = BrokerConfiguration()
+        configuration.load(["--server-autodiscover=false"])
+        self.assertEquals(configuration.server_autodiscover, False)

=== added file 'landscape/broker/tests/test_dnslookup.py'
--- landscape/broker/tests/test_dnslookup.py	1970-01-01 00:00:00 +0000
+++ landscape/broker/tests/test_dnslookup.py	2012-02-03 19:35:22 +0000
@@ -0,0 +1,199 @@
+from landscape.tests.helpers import LandscapeTest
+from landscape.broker.dnslookup import (lookup_server_record, lookup_hostname,
+                                        discover_server)
+
+from twisted.internet import defer
+from twisted.names import dns
+from twisted.names.error import ResolverError
+
+
+class FakeResolverResult(object):
+    """
+    A fake resolver result returned by L{FakeResolver}.
+
+    @param type: The result type L{twisted.names.dns.SRV}
+    @param payload: The result contents
+    """
+    def __init__(self):
+        self.type = None
+
+        class Payload(object):
+            """
+            A payload result returned by fake resolver.
+
+            @param target: The result of the lookup
+            """
+            def __init__(self):
+                self.target = ""
+
+        class Target(object):
+            """
+            A payload target returned by fake resolver.
+
+            @param name: The name contained by the target.
+            """
+            def __init__(self):
+                self.name = ""
+
+        self.payload = Payload()
+        self.payload.target = Target()
+
+
+class FakeResolver(object):
+    """
+    A fake resolver that mimics L{twisted.names.client.Resolver}
+    """
+    def __init__(self):
+        self.results = None
+        self.name = None
+
+    def lookupService(self, arg1):
+        deferred = defer.Deferred()
+        deferred.callback(self.results)
+        return deferred
+
+    def getHostByName(self, arg1):
+        deferred = defer.Deferred()
+        deferred.callback(self.name)
+        return deferred
+
+
+class BadResolver(object):
+    """
+    A resolver that mimics L{twisted.names.client.Resolver} and always returns
+    an error.
+    """
+    def lookupService(self, arg1):
+        deferred = defer.Deferred()
+        deferred.errback(ResolverError("Couldn't connect"))
+        return deferred
+
+    def getHostByName(self, arg1):
+        deferred = defer.Deferred()
+        deferred.errback(ResolverError("Couldn't connect"))
+        return deferred
+
+
+class DnsSrvLookupTest(LandscapeTest):
+    def test_with_server_found(self):
+        """
+        Looking up a DNS SRV record should return the result of the lookup.
+        """
+        fake_result = FakeResolverResult()
+        fake_result.type = dns.SRV
+        fake_result.payload.target.name = "a.b.com"
+        fake_resolver = FakeResolver()
+        fake_resolver.results = [[fake_result]]
+
+        def check(result):
+            self.assertEquals("a.b.com", result)
+
+        d = lookup_server_record(fake_resolver)
+        d.addCallback(check)
+        return d
+
+    def test_with_server_not_found(self):
+        """
+        Looking up a DNS SRV record and finding nothing exists should return
+        an empty string.
+        """
+        fake_resolver = FakeResolver()
+        fake_resolver.results = [[]]
+
+        def check(result):
+            self.assertEquals("", result)
+
+        d = lookup_server_record(fake_resolver)
+        d.addCallback(check)
+        return d
+
+    def test_with_resolver_error(self):
+        """A resolver error triggers error handling code."""
+        # The failure should be properly logged
+        logging_mock = self.mocker.replace("logging.info")
+        logging_mock("SRV lookup of _landscape._tcp.mylandscapehost.com "
+                     "failed.")
+        self.mocker.replay()
+
+        d = lookup_server_record(BadResolver())
+        self.assertFailure(d, ResolverError)
+        return d
+
+
+class DnsNameLookupTest(LandscapeTest):
+    def test_with_name_found(self):
+        """
+        Looking up a DNS name record should return the result of the lookup.
+        """
+        fake_resolver = FakeResolver()
+        fake_resolver.name = "a.b.com"
+
+        def check(result):
+            self.assertEquals("a.b.com", result)
+
+        d = lookup_hostname(None, fake_resolver)
+        d.addCallback(check)
+        return d
+
+    def test_with_name_not_found(self):
+        """
+        Looking up a DNS NAME record and not finding a result should return
+        None.
+        """
+        fake_resolver = FakeResolver()
+        fake_resolver.name = None
+
+        def check(result):
+            self.assertEquals(None, result)
+
+        d = lookup_hostname(None, fake_resolver)
+        d.addCallback(check)
+        return d
+
+    def test_with_resolver_error(self):
+        """A resolver error triggers error handling code."""
+        # The failure should be properly logged
+        logging_mock = self.mocker.replace("logging.info")
+        logging_mock("Name lookup of landscape.localdomain failed.")
+        self.mocker.replay()
+
+        d = lookup_hostname(None, BadResolver())
+        self.assertFailure(d, ResolverError)
+        return d
+
+
+class DiscoverServerTest(LandscapeTest):
+    def test_srv_lookup(self):
+        """The DNS name of the server is found using a SRV lookup."""
+        fake_result = FakeResolverResult()
+        fake_result.type = dns.SRV
+        fake_result.payload.target.name = "a.b.com"
+        fake_resolver = FakeResolver()
+        fake_resolver.results = [[fake_result]]
+
+        d = discover_server(fake_resolver)
+
+        def check(result):
+            self.assertEquals("a.b.com", result)
+
+        d.addCallback(check)
+        return d
+
+    def test_a_name_lookup(self):
+        """The DNS name of the server is found using an A name lookup."""
+        fake_resolver = FakeResolver()
+        fake_resolver.name = "x.y.com"
+
+        d = discover_server(fake_resolver)
+
+        def check(result):
+            self.assertEquals("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())
+        self.assertFailure(d, ResolverError)
+        return d

=== modified file 'landscape/broker/tests/test_ping.py'
--- landscape/broker/tests/test_ping.py	2011-07-05 05:09:11 +0000
+++ landscape/broker/tests/test_ping.py	2012-02-03 19:35:22 +0000
@@ -128,7 +128,7 @@
         self.url = "http://localhost:8081/whatever"
         self.page_getter = FakePageGetter(None)
 
-        def factory(reactor, url, insecure_id):
+        def factory(reactor, url, insecure_id, server_autodiscover):
             return PingClient(reactor, url, insecure_id,
                               get_page=self.page_getter.get_page)
         self.pinger = Pinger(self.broker_service.reactor,
@@ -201,7 +201,7 @@
 
         class BadPingClient(object):
             def __init__(self, *args, **kwargs):
-                pass
+                self.url = args[1]
 
             def ping(self):
                 return fail(ZeroDivisionError("Couldn't fetch page"))

=== modified file 'landscape/broker/tests/test_service.py'
--- landscape/broker/tests/test_service.py	2011-08-19 15:47:27 +0000
+++ landscape/broker/tests/test_service.py	2012-02-03 19:35:22 +0000
@@ -1,13 +1,12 @@
 import os
 
-from twisted.internet import reactor
-
 from landscape.tests.helpers import LandscapeTest
 from landscape.broker.tests.helpers import BrokerConfigurationHelper
 from landscape.broker.service import BrokerService
 from landscape.broker.transport import HTTPTransport
 from landscape.broker.amp import RemoteBrokerConnector
 from landscape.reactor import FakeReactor
+from twisted.internet import reactor
 
 
 class BrokerServiceTest(LandscapeTest):

=== modified file 'landscape/broker/tests/test_transport.py'
--- landscape/broker/tests/test_transport.py	2011-07-05 05:09:11 +0000
+++ landscape/broker/tests/test_transport.py	2012-02-03 19:35:22 +0000
@@ -52,11 +52,11 @@
 
     def test_get_url(self):
         url = "http://example/ooga"
-        transport = HTTPTransport(url)
+        transport = HTTPTransport(None, url)
         self.assertEqual(transport.get_url(), url)
 
     def test_set_url(self):
-        transport = HTTPTransport("http://example/ooga")
+        transport = HTTPTransport(None, "http://example/ooga")
         transport.set_url("http://example/message-system")
         self.assertEqual(transport.get_url(), "http://example/message-system")
 
@@ -71,7 +71,7 @@
         port = reactor.listenTCP(0, server.Site(r), interface="127.0.0.1")
         self.ports.append(port)
         transport = HTTPTransport(
-            "http://localhost:%d/" % (port.getHost().port,))
+            None, "http://localhost:%d/" % (port.getHost().port,))
         result = deferToThread(transport.exchange, "HI", computer_id="34",
                                message_api="X.Y")
 
@@ -98,7 +98,7 @@
                                  interface="127.0.0.1")
         self.ports.append(port)
         transport = HTTPTransport(
-            "https://localhost:%d/" % (port.getHost().port,), PUBKEY)
+            None, "https://localhost:%d/" % (port.getHost().port,), PUBKEY)
         result = deferToThread(transport.exchange, "HI", computer_id="34",
                                message_api="X.Y")
 
@@ -126,7 +126,7 @@
         port = reactor.listenSSL(0, server.Site(r), context_factory,
                                  interface="127.0.0.1")
         self.ports.append(port)
-        transport = HTTPTransport("https://localhost:%d/"
+        transport = HTTPTransport(None, "https://localhost:%d/"
                                   % (port.getHost().port,),
                                   pubkey=PUBKEY)
 
@@ -153,7 +153,7 @@
             return "filename"
         recorder.get_payload_filename = static_filename
 
-        transport = HTTPTransport("http://localhost",
+        transport = HTTPTransport(None, "http://localhost",
                                   payload_recorder=recorder)
 
         def fake_curl(param1, param2, param3):
@@ -174,7 +174,7 @@
         When C{HTTPTransport} is configured without a payload recorder,
         exchanges with the server should still complete.
         """
-        transport = HTTPTransport("http://localhost")
+        transport = HTTPTransport(None, "http://localhost")
         self.called = False
 
         def fake_curl(param1, param2, param3):

=== modified file 'landscape/broker/transport.py'
--- landscape/broker/transport.py	2011-07-21 23:55:47 +0000
+++ landscape/broker/transport.py	2012-02-03 19:35:22 +0000
@@ -5,12 +5,14 @@
 import pprint
 
 import pycurl
+from twisted.internet.threads import blockingCallFromThread
 
 from landscape.lib.fetch import fetch
 from landscape.lib.fs import create_file
 from landscape.lib import bpickle
 from landscape.log import format_delta
 from landscape import SERVER_API, VERSION
+from landscape.broker.dnslookup import discover_server
 
 
 class HTTPTransport(object):
@@ -22,10 +24,13 @@
         with the server.  If C{None}, exchanges will not be recorded.
     """
 
-    def __init__(self, url, pubkey=None, payload_recorder=None):
+    def __init__(self, reactor, url, pubkey=None, payload_recorder=None,
+                 server_autodiscover=False):
+        self._reactor = reactor
         self._url = url
         self._pubkey = pubkey
         self._payload_recorder = payload_recorder
+        self._server_autodiscover = server_autodiscover
 
     def get_url(self):
         """Get the URL of the remote message system."""
@@ -36,6 +41,14 @@
         self._url = url
 
     def _curl(self, payload, computer_id, message_api):
+        if self._server_autodiscover:
+            result = blockingCallFromThread(self._reactor, discover_server)
+            if result is not None:
+                self._url = "https://%s/message-system" % result
+            else:
+                logging.warn("Autodiscovery failed.  Falling back to previous "
+                             "settings.")
+
         headers = {"X-Message-API": message_api,
                    "User-Agent": "landscape-client/%s" % VERSION,
                    "Content-Type": "application/octet-stream"}
@@ -152,7 +165,8 @@
 class FakeTransport(object):
     """Fake transport for testing purposes."""
 
-    def __init__(self, url=None, pubkey=None, payload_recorder=None):
+    def __init__(self, reactor=None, url=None, pubkey=None,
+                 payload_recorder=None, server_autodiscover=False):
         self._pubkey = pubkey
         self._payload_recorder = payload_recorder
         self.payloads = []
@@ -163,6 +177,8 @@
         self.message_api = None
         self.extra = {}
         self._url = url
+        self._reactor = reactor
+        self._server_autodiscover = server_autodiscover
 
     def get_url(self):
         return self._url

=== modified file 'landscape/reactor.py'
--- landscape/reactor.py	2012-02-03 10:06:28 +0000
+++ landscape/reactor.py	2012-02-03 19:35:22 +0000
@@ -309,7 +309,7 @@
         self._LoopingCall = LoopingCall
         self._reactor = reactor
         self._cleanup()
-
+        self.callFromThread = reactor.callFromThread
         super(TwistedReactor, self).__init__()
 
     def _cleanup(self):

=== modified file 'landscape/tests/helpers.py'
--- landscape/tests/helpers.py	2011-07-28 12:54:34 +0000
+++ landscape/tests/helpers.py	2012-02-03 19:35:22 +0000
@@ -271,6 +271,7 @@
             "computer_title = Some Computer\n"
             "account_name = some_account\n"
             "ping_url = http://localhost:91910\n"
+            "server_autodiscover = false\n"
             "data_path = %s\n"
             "log_dir = %s\n" % (test_case.data_path, log_dir))
 
@@ -612,7 +613,7 @@
         if self._shared and self not in self.__class__._instances:
             self.__class__._instances.add(self)
         result.startTest(self)
-        if self.getSkip(): # don't run test methods that are marked as .skip
+        if self.getSkip():  # don't run test methods that are marked as .skip
             result.addSkip(self, self.getSkip())
             result.stopTest(self)
             return
@@ -643,6 +644,7 @@
 
 ### Copied from Twisted, to fix a bug in trial in Twisted 2.2! ###
 
+
 class UnsupportedTrialFeature(Exception):
     """A feature of twisted.trial was used that pyunit cannot support."""
 

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2012-01-11 16:33:15 +0000
+++ landscape/tests/test_configuration.py	2012-02-03 19:35:22 +0000
@@ -739,6 +739,7 @@
 computer_title = rex
 data_path = %s
 account_name = account
+server_autodiscover = False
 """ % config.data_path)
 
     def test_silent_setup_no_register(self):
@@ -755,6 +756,7 @@
         self.assertConfigEqual(self.get_content(config), """\
 [client]
 url = https://landscape.canonical.com/message-system
+server_autodiscover = False
 data_path = %s
 """ % config.data_path)
 
@@ -794,6 +796,7 @@
             "computer_title = \n"
             "https_proxy = \n"
             "ping_url = http://landscape.canonical.com/ping\n"
+            "server_autodiscover = False\n"
              % config.data_path)
 
     def test_silent_setup_without_computer_title(self):
@@ -875,7 +878,8 @@
              "computer_title": "rex",
              "include_manager_plugins": "ScriptExecution",
              "script_users": "root, nobody",
-             "account_name": "account"},
+             "account_name": "account",
+             "server_autodiscover": "False"},
             dict(parser.items("client")))
 
     def test_silent_script_users_with_all_user(self):
@@ -923,7 +927,8 @@
              "ping_url": "http://localhost/ping",
              "random_key": "random_value",
              "computer_title": "rex",
-             "account_name": "account"},
+             "account_name": "account",
+             "server_autodiscover": "False"},
             dict(parser.items("client")))
 
     def test_setup_with_proxies_from_environment(self):
@@ -975,7 +980,8 @@
              "http_proxy": "http://environ",
              "https_proxy": "https://environ",
              "computer_title": "rex",
-             "account_name": "account"},
+             "account_name": "account",
+             "server_autodiscover": "False"},
             dict(parser.items("client")))
 
     def test_setup_prefers_proxies_from_config_over_environment(self):
@@ -1241,7 +1247,8 @@
                           "registration_password": "New Password",
                           "http_proxy": "http://new.proxy",
                           "https_proxy": "https://new.proxy",
-                          "url": "http://new.url"})
+                          "url": "http://new.url",
+                          "server_autodiscover": "False"})
 
     def test_import_from_empty_file(self):
         self.mocker.replay()
@@ -1352,7 +1359,8 @@
                           "registration_password": "Command Line Password",
                           "http_proxy": "http://old.proxy",
                           "https_proxy": "https://old.proxy",
-                          "url": "http://new.url"})
+                          "url": "http://new.url",
+                          "server_autodiscover": "False"})
 
     def test_import_from_file_may_reset_old_options(self):
         """
@@ -1393,7 +1401,8 @@
                          {"computer_title": "Old Title",
                           "account_name": "Old Name",
                           "registration_password": "",  # <==
-                          "url": "http://old.url"})
+                          "url": "http://old.url",
+                          "server_autodiscover": "False"})
 
     def test_import_from_url(self):
         sysvconfig_mock = self.mocker.patch(SysVConfig)
@@ -1434,7 +1443,8 @@
                           "registration_password": "New Password",
                           "http_proxy": "http://new.proxy",
                           "https_proxy": "https://new.proxy",
-                          "url": "http://new.url"})
+                          "url": "http://new.url",
+                          "server_autodiscover": "False"})
 
     def test_import_from_url_with_http_code_fetch_error(self):
         fetch_mock = self.mocker.replace("landscape.lib.fetch.fetch")

=== modified file 'root-client.conf'
--- root-client.conf	2009-03-16 10:30:07 +0000
+++ root-client.conf	2012-02-03 19:35:22 +0000
@@ -11,3 +11,4 @@
 ping_url = http://localhost:8081/ping
 include_manager_plugins = ScriptExecution
 script_users = www-data, nobody, root
+