530975 - Some automated sanity checks: is subdomain, connection possible

Status: RESOLVED FIXED

(Webtools :: ISPDB Server, defect, P3)

Description

[Blake Winton (:bwinton) (:☕️)]

The checks to run for this first cut are:
a) domains are available.
b) we can establish a cxn with the config settings.
c) domains are sub-domains of the email domain.

Note: The reviewer can ignore the warnings, if there are any, at their discretion.  (i.e. we don't have to worry about GoogleApps domains not being sub-domains of googlemail.com.)

Comment 1

[Blake Winton (:bwinton) (:☕️)]

From DavidA over irc:
4) do a dns mx record lookup.

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: A patch to let the reviewer run some sanity checks.

Apparently we need PyDNS to do DNS lookups in Python, so I added it as a requirement.

I wasn't entirely sure what checks I should do with the MX record, David, so if you wanted to elaborate on those a little, that would be cool.

Thanks,
Blake.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: A better patch, which checks for errors, and doesn't hit the network.

So, I was running the tests for this patch, and they started giving me more errors than they used to, because something changed in one of the hosts I was using.  So I changed the tests to use mock objects for the DNS and socket calls, so that we can test those things even when we're disconnnected.

And then I added some client-side error handling, for the cases where the server doesn't respond for some reason.

Thanks,
Blake.

Comment 4

[Jeff Balogh (:jbalogh)]

Comment on attachment 416443 [details] [diff] [review]
A better patch, which checks for errors, and doesn't hit the network.

I can't argue with 200 lines of tests.

> -    pip install django python-openid django-openid-auth django-nose lxml
> +    pip install django python-openid django-openid-auth django-nose lxml PyDNS mox

pip install -r requirements.txt?

> +def sanity(request, id):
> +    config = Config.objects.filter(id=int(id))[0]

No get_object_or_404?  Are you using filter()[0] instead of get() for a reason?

> +    errors = []
> +    warnings = []
> +    for domain in Domain.objects.filter(config=config):
> +        host = domain.name
> +        inHost = config.incoming_hostname

What is this camelCase thing you're doing here?

> +        outHost = config.outgoing_hostname
> +
> +        # Check the DNS MX and SRV records.
> +        mxs = DNS.mxlookup(host.encode("utf-8"))
> +        if len(mxs) < 1:
> +          errors.append(u"Couldn’t find MX record for %s." % (host,))
> +
> +        result = check_srv_record(host, config.incoming_type,
> +                                  inHost, config.incoming_port)
> +        errors.extend(result["errors"])
> +        warnings.extend(result["warnings"])

This repeated extending makes me slightly uncomfortable, but I don't have a
great suggestion for a better way.  My first idea is something like

    def tally(result):
        errors.extend(result)
        warnings.extend(result)

as an inner function.

> +# Utility methods
> +
> +def check_connection(host, port):
> +    """Test to see that we can find and make a connection to the host and
> +       port passed in."""
> +
> +    errors = []
> +    warnings = []
> +
> +    # Make sure we can get an address for the host.
> +    addrinfo = None
> +    try:
> +        addrinfo = socket.getaddrinfo(host, None)
> +    except socket.gaierror, e:
> +        errors.append(u"Couldn’t get address for %s." % (host,))
> +
> +    # See if we can connect to the requested port.
> +    if addrinfo and len(addrinfo) > 0:

Isn't the second check redundant since "" evaluates to False?

> +def check_srv_record(domain, service, host, port):
> +    """Check that, if there are SRV records for this service and this
> +       domain, the passed-in host and port are somewhere in those SRV
> +       records."""
> +
> +    errors = []
> +    warnings = []
> +
> +    dnsReq = u"_%s._tcp.%s" % (service, domain)
> +    #dnsReq = u"_%s._tcp.%s" % (service, u"fudo.org")

Do you still need this line?

> +    dnsReq = dnsReq.encode("utf-8")
> +    a = DNS.Request(dnsReq, qtype = "srv").req().answers
> +    srv = [x["data"] for x in a if x["typename"] == "SRV"]
> +    srv.sort()
> +    if len(srv) > 0:
> +        valid = False
> +        records = []
> +        for record in srv:
> +            records.append("(%s:%d)" % (record[3], record[2]))

Does this need to be a u"" string?

> +            if port == record[2] and host == record[3]:
> +                valid = True
> +                break
> +        if not valid:
> +            errors.append(u"Couldn’t find settings (%s:%d) in %s SRV "
> +                          u"records [%s] for %s." % (
> +                          host, port, service, ", ".join(records), domain))

Can you use a for-else instead of managing the valid boolean?  If you break out
of the for loop, the else won't run, so you could append an error inside the
else block.

> diff --git a/templates/config/details.html b/templates/config/details.html
> +  $("#sanity_link").click(function() {

If your function takes the event argument and runs ``event.preventDefault()``,
the URL won't get the # appended.

> +    if ($(this).text() == "Running sanity checks.")
> +      return;
> +    $(this).html("Running sanity checks.");
> +    self = $(this);
> +    $.getJSON("{% url ispdb_sanity id=config.id %}", null,
> +      function(data) {
> +        self.html("Re-run sanity checks.");
> +        results = "";
> +        $.each(data["errors"],
> +          function(index, error) {
> +            results += '<span class="error">' + error + "</span><br/>";

+= has poor performance in js, similar to python.  Of course, you have < 10
strings here, so no one's going to notice.

> diff --git a/tests/test_sanity.py b/tests/test_sanity.py

The mocking here is pretty cool.  Verbose, but cool. I wish it wasn't copied
from a jaVaLibRary.

> +    def test_no_srv_errors(self):

Does this test differ significantly from test_no_errors?

Comment 5

[Ben Bucksch (:BenB)]

I'll narrow the subject to not overlap with other sanity checks and focus on the patch here. Bug 543614 is the tracker for sanity checks.

Comment 6

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with jbalogh's changes.

(In reply to comment #4)
> I can't argue with 200 lines of tests.

I think you could.  ;)

> > -    pip install django python-openid django-openid-auth django-nose lxml
> > +    pip install django python-openid django-openid-auth django-nose lxml PyDNS mox
> pip install -r requirements.txt?

Changed.

> > +    config = Config.objects.filter(id=int(id))[0]
> No get_object_or_404?  Are you using filter()[0] instead of get() for a reason?

Fixed.

> > +    errors = []
> > +    warnings = []
> > +    for domain in Domain.objects.filter(config=config):
> > +        host = domain.name
> > +        inHost = config.incoming_hostname
> What is this camelCase thing you're doing here?

Sorry, java habits.  Fixed.

> > +        result = check_srv_record(host, config.incoming_type,
> > +                                  inHost, config.incoming_port)
> > +        errors.extend(result["errors"])
> > +        warnings.extend(result["warnings"])
> This repeated extending makes me slightly uncomfortable, but I don't have a
> great suggestion for a better way.  My first idea is something like
>     def tally(result):
>         errors.extend(result)
>         warnings.extend(result)
> as an inner function.

That's not bad…  Fixed.

> > +    if addrinfo and len(addrinfo) > 0:
> Isn't the second check redundant since "" evaluates to False?

Fixed.

> > +    dnsReq = u"_%s._tcp.%s" % (service, domain)
> > +    #dnsReq = u"_%s._tcp.%s" % (service, u"fudo.org")
> Do you still need this line?

Nope.  Fixed.

> > +        for record in srv:
> > +            records.append("(%s:%d)" % (record[3], record[2]))
> Does this need to be a u"" string?

Apparently not, but fixed anyways.  :)

> > +            if port == record[2] and host == record[3]:
> > +                valid = True
> > +                break
> > +        if not valid:
> > +            errors.append(u"Couldn’t find settings (%s:%d) in %s SRV "
> > +                          u"records [%s] for %s." % (
> > +                          host, port, service, ", ".join(records), domain))
> Can you use a for-else instead of managing the valid boolean?  If you break out
> of the for loop, the else won't run, so you could append an error inside the
> else block.

Yeah, I never remember about the for-else.  It just seems slightly odd to me.  Anyways, fixed.

> > diff --git a/templates/config/details.html b/templates/config/details.html
> > +  $("#sanity_link").click(function() {
> If your function takes the event argument and runs ``event.preventDefault()``,
> the URL won't get the # appended.

Fixed.

> > +        $.each(data["errors"],
> > +          function(index, error) {
> > +            results += '<span class="error">' + error + "</span><br/>";
> += has poor performance in js, similar to python.  Of course, you have < 10
> strings here, so no one's going to notice.

What do you think about:
+        if (data["errors"].length) {
+          results.push('<span class="error">' +
+              data["errors"].join('</span><br/><span class="error">') +
+              "</span><br/>");
+        }
:)

> > diff --git a/tests/test_sanity.py b/tests/test_sanity.py
> The mocking here is pretty cool.  Verbose, but cool. I wish it wasn't copied
> from a jaVaLibRary.

Yeah.  I don't mind so much, since mocking seems kinda Java-y to me, but yeah.

> > +    def test_no_srv_errors(self):
> Does this test differ significantly from test_no_errors?

Yeah, in this test, the domain has SRV records, which test_no_errors doesn't.

Thanks,
Blake.

Comment 7

[Jeff Balogh (:jbalogh)]

Comment on attachment 429208 [details] [diff] [review]
The previous patch, with jbalogh's changes.

Discussed a couple things over IRC, looks good to me.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 429208 [details] [diff] [review]
The previous patch, with jbalogh's changes.

And over to Gozer for the second review.

Comment 9

[Philippe M. Chiasson (:gozer)]

(In reply to comment #1)
> From DavidA over irc:
> 4) do a dns mx record lookup.

Not sure what good dns MX records will do you. An ISP can very well have completely different smtp servers for who wants to send e-mail *in* and those who are sending e-mail *out*.

Comment 10

[Ben Bucksch (:BenB)]

However, if flintstones.com points to mx.dreamhost.com, then pop.dreamhost.com is probably OK as mail server for flintstones.com. However, pop.badguy.com is not.
In other words, a passing MX test would accept some otherwise rejected configs, it would not reject otherwise accepted configs

And as bwinton said:
> The reviewer can ignore the warnings, if there are any, at their discretion.

Comment 11

[Philippe M. Chiasson (:gozer)]

(In reply to comment #10)
> However, if flintstones.com points to mx.dreamhost.com, then pop.dreamhost.com
> is probably OK as mail server for flintstones.com. However, pop.badguy.com is
> not.
> In other words, a passing MX test would accept some otherwise rejected configs,
> it would not reject otherwise accepted configs

Works for me.

Comment 12

[Sergio Roberto]

This has been fixed in https://github.com/mozilla/ispdb/pull/7