Opened 8 years ago

Closed 7 years ago

#1461 closed task (fixed)

Implement DDNS system tests

Reported by: jelte Owned by: muks
Priority: medium Milestone: Sprint-20120619
Component: ddns Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: DDNS
Estimated Difficulty: 7 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

Implement the system tests from #1460.

We should use the Lettuce framework for this (and extend it where necessary).

Depending on the end result of #1460, this may end up being a meta-ticket for several smaller tickets. As such it may be hard to estimate a time for this.

Subtickets

Change History (17)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 7

comment:2 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Next-Sprint-Proposed

comment:3 Changed 8 years ago by jinmei

  • Milestone set to Next-Sprint-Proposed

comment:4 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120612

comment:5 follow-up: Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

Can we depend on nsupdate being present, or shall we create update packets on the fly?

I propose we at least split it into two parts; the main system tests and the protocol compliance tests.

I'll make a start with the former for this ticket. If this splitup seems OK, I'll create a followup ticket for the rest.

comment:6 in reply to: ↑ 5 Changed 7 years ago by jinmei

Replying to jelte:

Can we depend on nsupdate being present, or shall we create update packets on the fly?

I propose we at least split it into two parts; the main system tests and the protocol compliance tests.

I'll make a start with the former for this ticket. If this splitup seems OK, I'll create a followup ticket for the rest.

I think it makes sense. It'd be nicer to have workable system level
tests sooner especially because we currently don't have any, than
trying to have a more comprehensive test at once.

comment:7 follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Right, so i made test for the first 2 test sets (the system ones), and an outline for the third, but it needs better xfrout-notify control.

I have taken a look at the compliance tests as well, but for those we are going to need a custom update tool, since a lot of them require malformed updates that nsupdate will not send :)

Anyway, changes in trac1461, i made one more general change in the lettuce setup, bind10 is now always invoked with -n (no hotspot cache). I thought about making it optional but at this time I could not think of much reasons not to set it.

I'll create a ticket for the tool and compliance tests, perhaps said tool can also be a base for our own nsupdate implementation, should we want one.

comment:8 Changed 7 years ago by jinmei

You seem to forget committing or pushing something named "ddns.config.orig".

comment:9 Changed 7 years ago by jinmei

  • Owner changed from UnAssigned to jelte

comment:10 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned

arg, added.

comment:11 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks

comment:12 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to jelte

Hi jelte

  1. The config files should be formatted into multi-line ones (just like in the other lettuce tests). This makes both the file and future diffs easier to read.
  1. The following are perhaps best moved to bind10_control.py:
+@step('Configure BIND10 to run DDNS')
+@step('Configure BIND10 to stop running DDNS')
  1. There's a trailing newline added at the end of example.feature.
  1. This can't be fixed, right?
        # Note: test spec says refused here, system returns SERVFAIL
        #The DDNS response should be REFUSED
        The DDNS response should be SERVFAIL
  1. DDNS_RUNNING should perhaps be changed to DDNS_STARTED to make it consistent with other *_STARTED ids. This is a lettuce ticket, but maybe you can throw that in as it's a minor change and used here anyway.
  1. If prepare_update() accepts a zone name, maybe set_serial_to() should too.. but up to you :)
  1. Does this sequence log any messages to indicate that the ACL was updated? If so, we should wait for it to avoid a race. If not, we should log and wait for it.
        # Test 4
        When I send bind10 the following commands
        """
        config add DDNS/zones
        config set DDNS/zones[0]/origin example.org
        config add DDNS/zones[0]/update_acl {"action": "ACCEPT", "from": "127.0.0.1"}
        config commit
        """
  1. Even here, it would be good to doubly check any log message output by b10-ddns for the update. The nsupdate return is sufficient, but the log check would indicate our code in the particular handler was invoked and also avoid a race with nsupdate.
    	# Test 5
    	When I use DDNS to set the SOA serial to 1237
    	The DDNS response should be SUCCESS
    	And the SOA serial for example.org should be 1237
    
  1. Is DDNS restarted automatically after the shutdown? In this case, please add a comment between the two lettuce statements:
    	# Test 6
    	When I send bind10 the command DDNS shutdown
    
    +       # Perhaps add this too:
    +       And wait for new bind10 stderr message DDNS_STOPPED
    +
    +       # DDNS is now automatically restarted by BoB.
    +
    	# Test 7
    	And wait for new bind10 stderr message DDNS_RUNNING
    
    
  1. Is this due to a race in the next lettuce command getting to DDNS before it's ready?
	# Test 8
	# Known issue: after shutdown, first new attempt results in SERVFAIL
        When I use DDNS to set the SOA serial to 1238
	The DDNS response should be SERVFAIL
	And the SOA serial for example.org should be 1237
  1. Same as item 9 here:
    	# Test 9
    	When I send bind10 the command Auth shutdown
    	And wait for new bind10 stderr message AUTH_SERVER_STARTED
    
  1. Does update_acl allow IP addresses only? Are hostnames not allowed? Also, why is it called update_acl and not just acl ? (Sorry about the silly questions, but I'm asking out of ignorance).
    @step('set DDNS ACL ([0-9]+) for ([0-9.]+) to ([A-Z]+)')
    def set_ddns_acl_to(step, nr, address, action):
    

Back to you :)

comment:13 in reply to: ↑ 12 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

Replying to muks:

Hi jelte

  1. The config files should be formatted into multi-line ones (just like in the other lettuce tests). This makes both the file and future diffs easier to read.

oh right, i tend not to use the files directly :) done

  1. The following are perhaps best moved to bind10_control.py:
+@step('Configure BIND10 to run DDNS')
+@step('Configure BIND10 to stop running DDNS')

ok, moved

  1. There's a trailing newline added at the end of example.feature.

oops, removed

  1. This can't be fixed, right?
        # Note: test spec says refused here, system returns SERVFAIL
        #The DDNS response should be REFUSED
        The DDNS response should be SERVFAIL

I think it can, but it would be out of scope for this ticket.

  1. DDNS_RUNNING should perhaps be changed to DDNS_STARTED to make it consistent with other *_STARTED ids. This is a lettuce ticket, but maybe you can throw that in as it's a minor change and used here anyway.

Yes, tests like these make such things quite obvious, don't they :)

I originally considered it out of scope as well, but it's too small to spend a new ticket on, changed.

  1. If prepare_update() accepts a zone name, maybe set_serial_to() should too.. but up to you :)

easy to add if we want it, skipping for now

  1. Does this sequence log any messages to indicate that the ACL was updated? If so, we should wait for it to avoid a race. If not, we should log and wait for it.
        # Test 4
        When I send bind10 the following commands
        """
        config add DDNS/zones
        config set DDNS/zones[0]/origin example.org
        config add DDNS/zones[0]/update_acl {"action": "ACCEPT", "from": "127.0.0.1"}
        config commit
        """

nothing is logged at this point, but commit shouldn't return before the module has processed the update anyway, so I do not think we can have a race here. We might still want to add a lowlevel debug statement for administrators convenience, but I do not think it is necessary here (so I'm considering this out of scope for at least this ticket, feel free to suggest it as an enhancement).

  1. Even here, it would be good to doubly check any log message output by b10-ddns for the update. The nsupdate return is sufficient, but the log check would indicate our code in the particular handler was invoked and also avoid a race with nsupdate.
    	# Test 5
    	When I use DDNS to set the SOA serial to 1237
    	The DDNS response should be SUCCESS
    	And the SOA serial for example.org should be 1237
    

I've added one that could in theory be a problem right now (and fixed a test prerequisite; the module tests were supposed to serve the zone from memory loaded from db); it does wait for the message that the zone has been reloaded into memory. In all the other cases I again think that there should be no race; ddns wouldn't respond until the transaction was done. (also, I don't think there is a separate 'ddns handled and done, all good' log message, unless you count DDNS_UPDATE_NOTIFY. Same as for updating acl; feel free to suggest enhancement :)

  1. Is DDNS restarted automatically after the shutdown? In this case, please add a comment between the two lettuce statements:
    	# Test 6
    	When I send bind10 the command DDNS shutdown
    
    +       # Perhaps add this too:
    +       And wait for new bind10 stderr message DDNS_STOPPED
    +
    +       # DDNS is now automatically restarted by BoB.
    +
    	# Test 7
    	And wait for new bind10 stderr message DDNS_RUNNING
    
    

ok, added

  1. Is this due to a race in the next lettuce command getting to DDNS before it's ready?
	# Test 8
	# Known issue: after shutdown, first new attempt results in SERVFAIL
        When I use DDNS to set the SOA serial to 1238
	The DDNS response should be SERVFAIL
	And the SOA serial for example.org should be 1237

No, it has to do with the communication from auth to ddns, see http://bind10.isc.org/ticket/1460#comment:9

  1. Same as item 9 here:
    	# Test 9
    	When I send bind10 the command Auth shutdown
    	And wait for new bind10 stderr message AUTH_SERVER_STARTED
    

added

  1. Does update_acl allow IP addresses only? Are hostnames not allowed? Also, why is it called update_acl and not just acl ? (Sorry about the silly questions, but I'm asking out of ignorance).
    @step('set DDNS ACL ([0-9]+) for ([0-9.]+) to ([A-Z]+)')
    def set_ddns_acl_to(step, nr, address, action):
    

I don't know, I just write the tests ;)

I suspect it is because it may at some point be converged with other acl's, and hence need to be clearly differentiated from, say, query_acl (and/or to make it at least sound a bit like the corresponding bind 9 value).

localhost probably also works, but i tend to prefer 127.0.0.1 over localhost, tbh :)

comment:14 Changed 7 years ago by muks

I think this bug is ready for merge now. Because Jelte is not around, I've been assigned the merge task (on Jabber).

Before this bug is resolved, a new ticket has to be created for the tasks in comment 7.

comment:15 Changed 7 years ago by muks

I've added a minor patch to start zonemgr:

* 38cf088[1461] DDNS (when started later in the test) still requires zonemgr

comment:16 in reply to: ↑ 7 Changed 7 years ago by muks

Replying to jelte:

Right, so i made test for the first 2 test sets (the system ones), and an outline for the third, but it needs better xfrout-notify control.

I have taken a look at the compliance tests as well, but for those we are going to need a custom update tool, since a lot of them require malformed updates that nsupdate will not send :)

Anyway, changes in trac1461, i made one more general change in the lettuce setup, bind10 is now always invoked with -n (no hotspot cache). I thought about making it optional but at this time I could not think of much reasons not to set it.

I'll create a ticket for the tool and compliance tests, perhaps said tool can also be a base for our own nsupdate implementation, should we want one.

Bug #2061 has been created for it.

comment:17 Changed 7 years ago by muks

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

Merged to master in commmit 97c1c1f96301a48eb6ed3fd10201e188b6cef6ba:

* f2cd12c [1461] add more log checks to ddns systest
* c54635a [1461] move start/stop ddns steps to bind10_control.py
* fa2055d [1461] make test config files multiline
* 9ac04cd [1461] add missing config input file for ddns test
* 6e0c6be [1461] docstrings
* ca65e4d [1461] add commented-out ddns-xfrout test
* fbb8d8a [1461] DDNS test set 2
* ee56de1 [1461] complete first test set
* 14181ee [1461] cleaner step definitions
* e8df143 [1461] additional tests from set 1
* dd8a0ae [1461] initial startup test
* 7087a74 [1461] initial addition of config for tests

Merged separately:

* 38cf088 [1461] DDNS (when started later in the test) still requires zonemgr

Resolving as fixed on behalf of Jelte.

Note: See TracTickets for help on using tickets.