Opened 10 years ago

Closed 9 years ago

#167 closed task (fixed)

review: configure addresses and ports to listen on for DNS servers

Reported by: larissas Owned by: each
Priority: medium Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description

Way to configure addresses and ports to listen on for DNS servers. Adding address/port tuples for services (auth, etc). port selection to specify one address to listen to a service (procedure for using services available now, but other services can be plugged in later)

(2 days)
(backlog task 43)

Subtickets

Attachments (1)

167.diff (16.9 KB) - added by each 9 years ago.
diff against current trunk

Download all attachments as: .zip

Change History (29)

comment:1 follow-up: Changed 10 years ago by each

  • Owner changed from each to UnAssigned
  • Status changed from new to reviewing

Please review:
svn diff -r 1879:HEAD branches/trac167

Note that this was branched from branches/trac168, which has not been reviewed/merged yet.

comment:2 in reply to: ↑ 1 ; follow-up: Changed 10 years ago by jinmei

Replying to each:

Please review:
svn diff -r 1879:HEAD branches/trac167

Note that this was branched from branches/trac168, which has not been reviewed/merged yet.

There don't seem to be tests for this feature. (I know this is probably a short lived feature, but IMO we shouldn't use it as an excuse for not testing it)

And, assuming we need tests but don't have them, it indicates the TDD hasn't been used for this feature:-)

comment:3 in reply to: ↑ 2 ; follow-up: Changed 10 years ago by each

There don't seem to be tests for this feature. (I know this is probably a short lived feature, but IMO we shouldn't use it as an excuse for not testing it)

No, true. There aren't any tests for the -p option either.

I'm not actually sure how to use our unit test framework to test "it's listening only on the specified address and port". I can test that it rejects invalid addresses, but is there a way to trigger incoming connections and confirm that they don't work when they aren't supposed to?

comment:4 in reply to: ↑ 3 ; follow-ups: Changed 10 years ago by jinmei

Replying to each:

There don't seem to be tests for this feature. (I know this is probably a short lived feature, but IMO we shouldn't use it as an excuse for not testing it)

No, true. There aren't any tests for the -p option either.

I'm not actually sure how to use our unit test framework to test "it's listening only on the specified address and port". I can test that it rejects invalid addresses, but is there a way to trigger incoming connections and confirm that they don't work when they aren't supposed to?

"only this" may be difficult to test, but we can probably confirm the testee actually tries to bind a socket to a specific address[:port] by letting the tester make that exact binding first and seeing the subsequent attempt fail:

in the test code:

   s.bind("[::1]:5300")
   assertRaises(socket_error_or_something, testee.do_bind("::1", 5300))

etc.

Another possibility is to separate the actual socket related operations and option handling clearly, and to make sure at least the options are interpreted as expected.

btw, in my understanding one of the (advocated) TDD is that by considering how to test it before even designing it we can improve t testing the feature doesn't seem to be straightforward. (Of course, on the other hand, tweaking the design for the convenice of some very tricky test cases could rather damage the design. So we should be careful not being too tricky, but I think we should try hard by default designing things so that they can be tested)

Oh, btw, another unrelated comment I happen to notice: I suggest we use socket.getaddrinfo() instead of socket.inet_pton(). Whether or not we want to support IPv8:-), I believe we'd like to avoid this type of exception usage:

+            a = socket.inet_pton(f, addr)
+        except socket.error:
+            try:
+                f = socket.AF_INET6
+                a = socket.inet_pton(f, addr)
+            except socket.error as se:
+                raise se

(clearly this abuses an exception because the first exeption is not actually an exceptional case)

comment:5 in reply to: ↑ 4 Changed 10 years ago by jinmei

Replying to jinmei:

btw, in my understanding one of the (advocated) TDD is that by considering how to test it before even designing it we can improve t testing the feature doesn't seem to be straightforward.

hmm...this part seems to be somehow broken...I should have checked it before submitting the comment. Sorry about the garbage, what I meant was:

in my understanding one of the (advocated) TDD advantages is that by considering how to test it before even designing it we can improve the modularity especially when testing the feature doesn't seem to be straightforward.

comment:6 in reply to: ↑ 4 ; follow-up: Changed 10 years ago by each

Oh, btw, another unrelated comment I happen to notice: I suggest we use socket.getaddrinfo() instead of socket.inet_pton(). Whether or not we want to support IPv8:-), I believe we'd like to avoid this type of exception usage:

It's my impression that python rather encourages this usage; anyway there are lots of examples of it in the various pythonic code-recipe websites and books I've looked at. I know we want to avoid routine exceptions in C++ for performance reasons, but I'm not sure it's a problem in python (especially in this case, an input-validation routine).

I chose to use inet_pton() instead of getaddrinfo() because I only wanted to it parse IP addresses, but getaddrinfo() will also look up domain names. Do you know if there's a way to make getaddrinfo() parse addresses only? Or some other way to accomplish the same thing?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 10 years ago by jinmei

Replying to each:

I chose to use inet_pton() instead of getaddrinfo() because I only wanted to it parse IP addresses, but getaddrinfo() will also look up domain names. Do you know if there's a way to make getaddrinfo() parse addresses only? Or some other way to accomplish the same thing?

Use AI_NUMERICHOST.

        addrinfo = socket.getaddrinfo(addrstr, portstr, socket.AF_UNSPEC,
                                      socket.SOCK_STREAM, socket.IPPROTO_TCP,
                                      socket.AI_NUMERICHOST|
                                      socket.AI_NUMERICSERV)

(unfortunately these flags don't seem to be documented, which is bad)

You can also see a usage example in the latest version of xfrin under the trac185 branch.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 10 years ago by each

Use AI_NUMERICHOST.

Hmm. Thank you, that does work, but it's somewhat more accepting than I wanted (for example, it will happily accept an IP address of '2'), and the exception it throws when it sees an invalid IP address is the counterintuitive "Name or service not known".

That would be fine if there was big performance win from avoiding the exception, but I don't think that's the case. For one thing we're only running this code during command line argument parsing, during which performance doesn't matter much. But more generally, a cascading series of "try...catch" statements seems to be very much the "pythonic" way of coding.

(See, for example, _Python 3 for Absolute Beginners_, p. 238: "Some langauges try to make exception handling a last resort -- the syntax can often be cumbersome and the functionality basic -- and the performance of the language interpreter might suffer during its equivalent of 'try' statements. But as you have seen, Python actively _encourages_ error handling, through simple syntax with a fully developed flow control based on exception handling. Also, in most Python distributions, exception handling is no slower than simple 'if...else' flow control.")

comment:9 in reply to: ↑ 8 Changed 9 years ago by jinmei

  • Owner changed from UnAssigned to each

Replying to each:

I was surprised that this is going to be a controversial discussion:-) I'm going to explain my points anyway, but I don't think this small topic is worth much time of discussion. If you are still not convinced after reading this, please simply ignore my comment on this point and go ahead with inet_pton..

Use AI_NUMERICHOST.

Hmm. Thank you, that does work, but it's somewhat more accepting than I wanted (for example, it will happily accept an IP address of '2'), and the exception it throws when it sees an invalid IP address is the counterintuitive "Name or service not known".

As for "more accepting", yes, I don't like that either but unfortunately the spec requires getaddrinfo() accept what inet_addr() would accept (BSD-derived implementations deliberately use stricter validation, but technically they are not spec conformant).

That would be fine if there was big performance win from avoiding the exception, but I don't think that's the case. For one thing we're only running this code during command line argument parsing, during which performance doesn't matter much. But more generally, a cascading series of "try...catch" statements seems to be very much the "pythonic" way of coding.

(See, for example, _Python 3 for Absolute Beginners_, p. 238: "Some langauges try to make exception handling a last resort -- the syntax can often be cumbersome and the functionality basic -- and the performance of the language interpreter might suffer during its equivalent of 'try' statements. But as you have seen, Python actively _encourages_ error handling, through simple syntax with a fully developed flow control based on exception handling. Also, in most Python distributions, exception handling is no slower than simple 'if...else' flow control.")

I'm not sure if the quoted part of text encourages cascading "try...catch" or it contradict my comment...when I said "his abuses an exception" I meant it's not "error handling" in the first place.

But in any case, the more serious issue in this specific case is that the cascading style (IMO) reduces code readability, whether it's a cascading try-except or nested if-else. The cascading currently reads:

try:
    f = socket.AF_INET
    a = socket.inet_pton(f, addr)
except socket.error:
    try:
        f = socket.AF_INET6
        a = socket.inet_pton(f, addr)
    except socket.error as se:
        raise se
except Exception as e:
    print(str(e))

Decent level programmers can probably figure out what this code does, but that wouldn't be immediately obvious. And, due to the nested conditions it's not so easy to be confident that this code does what it's supposed to do from a glance. Not at least to me - if I can claim I'm a decent level programmer.

On the other hand, if you write essentially the same logic with getaddrinfo, it would read as follows:

try:
    addrinfo = socket.getaddrinfo(addr, port, socket.AF_UNSPEC)
except socket.gaierror as err:
    raise err
except Exception as e:
    print(e)

since getaddrinfo implements all the complicated nested conditions inside, we can concentrate on the main logic here: accept any valid IPv4/IPv6 address.

BTW I didn't care about performance implication at all. I normally forget performance issues for something we (validly) decide to write in python.

comment:10 follow-up: Changed 9 years ago by each

Decent level programmers can probably figure out what this code does, but that wouldn't be immediately obvious. And, due to the nested conditions it's not so easy to be confident that this code does what it's supposed to do from a glance.

If nesting is the problem, there are other ways to structure it. Could have a series of try...except statements with "pass" in each of the excepts, for instance. Or this:

  for family in (socket.AF_INET, socket.AF_INET6):
    try:
      a = socket.inet_pton(family, addr)
      self.family = family
      self.addr = a
      return
    except socket.error:
      continue
    except Exception as e:
      raise e

    raise socket.error('Invalid IP address')

comment:11 in reply to: ↑ 10 Changed 9 years ago by jinmei

Replying to each:

Decent level programmers can probably figure out what this code does, but that wouldn't be immediately obvious. And, due to the nested conditions it's not so easy to be confident that this code does what it's supposed to do from a glance.

If nesting is the problem, there are other ways to structure it. Could have a series of try...except statements with "pass" in each of the excepts, for instance. Or this:

The real problem is complexity that makes the code difficult to understand. The nesting is just one source of the complexity. This one is better than the previous one, but is still complex. We need to follow the logic with the mixture of for, try, except, continue, raise, wondering "what if we hit the contine then an exception is thrown, is that safe?" etc.

  for family in (socket.AF_INET, socket.AF_INET6):
    try:
      a = socket.inet_pton(family, addr)
      self.family = family
      self.addr = a
      return
    except socket.error:
      continue
    except Exception as e:
      raise e

    raise socket.error('Invalid IP address')

I simply don't understand why you want to try so hard making the complex logic correct while you can do this with just a couple of lines with getaddrinfo. There should probably be something we never agree with, and, as I said, this wouldn't be that important anyway and we can use our time for something more important. So go ahead with whatever code you want (but please don't forget writing tests to make sure the complex logic does the right thing).

comment:12 Changed 9 years ago by shane

  • Component changed from Unclassified to b10-auth

comment:13 Changed 9 years ago by shane

  • Milestone changed from 04. 2nd Incremental Release: Early Adopters to 05. 3rd Incremental Release

comment:14 Changed 9 years ago by jreed

Need to merge the ASIO fixes into this branch.

session.cc:29:20: error: asio.hpp: No such file or directory

comment:15 Changed 9 years ago by each

  • Owner changed from each to UnAssigned

I've added tests to the bind10 module to confirm that address and port are set correctly in the BoB class and that the IPAddr constructor fails when I expect it to.

As Jeremy noted, this is going to need to be rebased with the ASIO changes, but it won't be difficult to do so.

Ready for re-review. I'm leaving it unassigned at the moment because I'm not sure whether Jinmei has time.

comment:16 Changed 9 years ago by each

  • Summary changed from configure addresses and ports to listen on for DNS servers to review: configure addresses and ports to listen on for DNS servers

comment:17 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:18 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to UnAssigned

Review of branches/trac167 revision 2268

Files modified/added since the branch was created at revision 1879
(output of svn diff -r 1879:HEAD --summarize).

M src/bin/auth/main.cc
M src/bin/bind10/tests/bind10_test.py
M src/bin/bind10/bind10.py.in

src/bin/auth/main.cc
General
The documentation in this module is sparse.

The opening brace of some functions is not on the end of the signature when the signature is on one line (BIND-10 C++ coding standards).

check_axfr_query
A C-style cast is used - the C++ style "xxx_cast<>()" is preferred.

A "magic" number (0xFC80) is hard-coded in the function.

The single-line "if" blocks should be surrounded by braces (BIND-10 C++ coding standards).

dispatch_axfr_query
A C-style cast is used - the C++ style "xxx_cast<>()" is preferred

class TCPClient
No indication given should an error occur; instead the object silently disappears.

class UDPServer
Does not appear to be any logging should an error occur in the receive function, nor if a zero-length message is received.

run_server()
There is no check that the "port" argument is valid. This function converts the passed string to "short" via a call to "atoi()", but atoi() makes no check as to the validity of the argument. Suggest that this could be checked via a call to boost::lexical_cast.

Remark: If the address argument does not match the flag -4/-6, an error message is output and the program terminated via a call to exit(). However, main() explicitly deletes the auth_server object before terminating. If this latter behaviour is desired in all cases, run_server should return on error instead of exiting.

Slight inconsistency: the first "if" checks "if (address != NULL)", the second "if" checks "if (address)". (In fact, the first clause of the second "if" could be combined into the first "if" construct.)

It appears that if "address" is given, the servers.xxx4_server variables are set regardless of whether the address is an IPV4 or IPV6 address.

main()
The "case" branch parsing of "-6" results includes what looks like temporary code 'cout << "here" << endl'.

If B10_FROM_SOURCE is defined, the path "/src/bin/auth/auth.spec" is appended to it to file the specification file. Just a bit unhappy that this path is buried deep in the code - it would be better either declared as a constant at the head of the file or placed into a separate header file.

src/bin/bind10/tests/bind10_test.py
TestProcessInfo
test_init
Should check that pi.dev_null_stderr is also set to False on default construction.

"env" is an argument to the ProcessInfo constructor. Why have the tests for the initialization of this member been commented out?

TestBob
Only checks that the arguments have been stored correctly. What tests are done that the other methods work?

src/bin/bind10/bind10.py.in
(Just a check of the changes since revision 1879)

Class !ProcessInfo
Remark: As this class creates a process, to my untutored eyes ProcessInfo seems a misleading name.

comment:19 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to each

comment:20 in reply to: ↑ 18 ; follow-up: Changed 9 years ago by each

Thank you for the review. As some of the comments were unrelated to the specifics of this ticket I have created a new cleanup ticket, #259, and moved the comments there. The items remaining that appear to me to be related to this specific change are these:

Replying to stephen:

run_server()
There is no check that the "port" argument is valid. This function converts the passed string to "short" via a call to "atoi()", but atoi() makes no check as to the validity of the argument. Suggest that this could be checked via a call to boost::lexical_cast.

Remark: If the address argument does not match the flag -4/-6, an error message is output and the program terminated via a call to exit(). However, main() explicitly deletes the auth_server object before terminating. If this latter behaviour is desired in all cases, run_server should return on error instead of exiting.

Slight inconsistency: the first "if" checks "if (address != NULL)", the second "if" checks "if (address)". (In fact, the first clause of the second "if" could be combined into the first "if" construct.)

It appears that if "address" is given, the servers.xxx4_server variables are set regardless of whether the address is an IPV4 or IPV6 address.

main()
The "case" branch parsing of "-6" results includes what looks like temporary code 'cout << "here" << endl'.

If I left out anything that you would like to see addressed in this ticket, let me know. Otherwise, I'll take care of these this afternoon.

comment:21 in reply to: ↑ 20 Changed 9 years ago by each

  • Owner changed from each to stephen

Replying to each:

If I left out anything that you would like to see addressed in this ticket, let me know. Otherwise, I'll take care of these this afternoon.

Addressed the above in r2336.

comment:22 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to each

Checked revision 2341:

1) Although the "if" blocks have been combined (in run_server), should an address is given the code will still set the servers.xxx4_server variables even if the address is an IPV6 address.

2) Entering an invalid port on the command line will result in the output of the somewhat cryptic message "bad lexical cast: source type value could not be interpreted as target". I would suggest that that exception is caught and re-thrown with a more user-friendly message.

3) Something I missed in the original review is that the numeric port number should be "unsigned short" all the way through the code (it is currently declared as "short").

comment:23 in reply to: ↑ 22 Changed 9 years ago by each

  • Owner changed from each to stephen

1) Although the "if" blocks have been combined (in run_server), should an address is given the code will still set the servers.xxx4_server variables even if the address is an IPV6 address.

Oops. Good catch.

2) Entering an invalid port on the command line will result in the output of the somewhat cryptic message "bad lexical cast: source type value could not be interpreted as target". I would suggest that that exception is caught and re-thrown with a more user-friendly message.

Done (but it was also validated in the bind10 python wrapper).

3) Something I missed in the original review is that the numeric port number should be "unsigned short" all the way through the code (it is currently declared as "short").

Addressed. OK to merge?

comment:24 follow-up: Changed 9 years ago by shane

  • Owner changed from stephen to each

This is okay to merge, although I have a question.

The run_server() function was changed, but no tests updated. Should we add a test to insure that an IPv6 address works? Also maybe we should add a test to check what is thrown if we pass an invalid port number?

comment:25 Changed 9 years ago by shane

Okay, Evan will merge the code - I have added the suggestions for more tests to ticket #259.

comment:26 in reply to: ↑ 24 Changed 9 years ago by jinmei

Commenting on something I just happen to notice:

Replying to shane:

This is okay to merge, although I have a question.

The run_server() function was changed, but no tests updated. Should we add a test to insure that an IPv6 address works? Also maybe we should add a test to check what is thrown if we pass an invalid port number?

(I'm not fully sure what an invalid port number means but) yes, we need these kinds of tests. I guess we should do this with refactoring the ASIO link (sub)module. (in trac #221 I've added some network level tests. but we'll also need whether it works as specified via configuration/comand line option)

Changed 9 years ago by each

diff against current trunk

comment:27 Changed 9 years ago by each

Replying to jinmei:

(I'm not fully sure what an invalid port number means but) yes, we need these kinds of tests. I guess we should do this with refactoring the ASIO link (sub)module. (in trac #221 I've added some network level tests. but we'll also need whether it works as specified via configuration/comand line option)

Agreed, but again, I'd prefer to merge what's been done and address this comment in #259.

Meanwhile, may I request a sanity check on the rebasing I've done? This ticket was started prior to the boost::asio->asio change, the creation of asio_link.cc, and msgq using a unix domain socket, and the changes are extensive enough to make me concerned I might have missed something in the merge. I've attached a diff against the current trunk.

comment:28 Changed 9 years ago by each

  • Resolution set to fixed
  • Status changed from reviewing to closed

On further thought, since we don't have an imminent release, I'll go ahead and commit this now. If there are errors in the merge I'll address them in #259.

Note: See TracTickets for help on using tickets.