Opened 8 years ago

Closed 7 years ago

#1514 closed task (fixed)

Update SERIAL in DDNS

Reported by: jelte Owned by: haikuo
Priority: medium Milestone: Sprint-20120612
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: 4 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

If a DDNS UPDATE does not change the SOA record, it should be done automatically (the serial field must be incremented). See http://bind10.isc.org/wiki/DDNSDesignDocument#IncrementingSOAserialvalue

Subtickets

Change History (20)

comment:1 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 8 years ago by jelte

  • Milestone set to Sprint-20120529

comment:3 follow-up: Changed 8 years ago by haikuo

  • Owner set to haikuo
  • Status changed from new to accepted

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

Replying to haikuo:

Note: this task may be affected by the result of #1512. If you've
not made much progress on this ticket, I'd suggest holding off
(and working on non DDNS tasks for now) until at least we agree on
the basic code structure of #1512, and then work on this task on top
of (merged or snapshot of) #1512.

But, of course, it's ultimately up to you.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 years ago by haikuo

Replying to jinmei:

Replying to haikuo:

Note: this task may be affected by the result of #1512. If you've
not made much progress on this ticket, I'd suggest holding off
(and working on non DDNS tasks for now) until at least we agree on
the basic code structure of #1512, and then work on this task on top
of (merged or snapshot of) #1512.

But, of course, it's ultimately up to you.

After my investigation, I agree with you, so this ticket can be postponed until #1512 is clear. But I am not familar at the functionality of DDNS now, so I will spend some time in reading more documents about the DDNS

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

Replying to haikuo:

Note: this task may be affected by the result of #1512. If you've
not made much progress on this ticket, I'd suggest holding off
(and working on non DDNS tasks for now) until at least we agree on
the basic code structure of #1512, and then work on this task on top
of (merged or snapshot of) #1512.

But, of course, it's ultimately up to you.

After my investigation, I agree with you, so this ticket can be postponed until #1512 is clear. But I am not familar at the functionality of DDNS now, so I will spend some time in reading more documents about the DDNS

Okay, I've not seen fundamental level controversy in #1512, so I think
you can resume this task anytime, based on the latest snapshot of
trac1512.

comment:7 Changed 8 years ago by haikuo

the unittest and the codes for the algorithm is done, and I am waitting for the ticket #1457 review, then I will update my codes to the snapshot of #1457

comment:8 Changed 8 years ago by haikuo

  • Owner changed from haikuo to UnAssigned
  • Status changed from accepted to reviewing

the codes is done, please review it.
this ticket is on the snapshot of #1457, and it should be merged into master only after #1457 is merged.

I modified one of unit test in the #1457, the serial number should be update autimatically in the update operation, but the unit test in #1457 is not. Jelter(the ower of #1457) has made sure that.

comment:9 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:10 follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to haikuo

session.py:

in the DDNS_SOA class, soa_update_check() and update_soa() have no docstrings, and the two other docstrings could probably use a bit of extra explanation about their arguments and what exactly they return.

Also, if the method of converting to string, doing magic on it, and converting back is the only way to do it in python, we need better (SOA) rdata support in the wrappers, or maybe even in libdns++. I'm not saying we should do that now, but it's worth mentioning in a comment there that this is a very bad workaround for an incomplete API :)

But given this code, there are a few improvements to make;

  • the for loop isn't necessary; you could just as easily do
    soa_text_parts = origin_soa.get_rdata()[0].to_text().split()
    soa_text_parts[2] = str(soa_num.get_value())
    new_soa.add_rdata(Rdata(origin_soa.get_type(), origin_soa.get_class(),
                      " ".join(soa_text_parts)))
    

However, this does make me wonder if a basic split() even always works...

  • Not sure whether it even needs to be a class, or only a few module-level functions; the class does not keep any state at all (so the only purpose seems to be to hide the first two methods)

I also have a few comments about the update_soa() changes;

  • it'll fail if you actually give it a soa with a lower serial (since new_soa won't be set). This btw also suggest that we need to have a unit test that tries that :)
  • the assignment of old to new isn't really necessary in the else-statement; doing the update_soa directly on old_soa should also work
  • both of the above can i think be fixed by merging the nested ifs; something like
    if added_soa is not None and serial_update_check:
      new_soa = added_soa
    else:
      new_soa = update_serial(old_soa)
    
    (names not reflecting actual current code)
    

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

  • Owner changed from haikuo to jelte

Replying to jelte:

session.py:

in the DDNS_SOA class, soa_update_check() and update_soa() have no docstrings, and the two other docstrings could probably use a bit of extra explanation about their arguments and what exactly they return.

sorry, forgot it. I have appended the comments for the function now.

Also, if the method of converting to string, doing magic on it, and converting back is the only way to do it in python, we need better (SOA) rdata support in the wrappers, or maybe even in libdns++. I'm not saying we should do that now, but it's worth mentioning in a comment there that this is a very bad workaround for an incomplete API :)

But given this code, there are a few improvements to make;

  • the for loop isn't necessary; you could just as easily do
    soa_text_parts = origin_soa.get_rdata()[0].to_text().split()
    soa_text_parts[2] = str(soa_num.get_value())
    new_soa.add_rdata(Rdata(origin_soa.get_type(), origin_soa.get_class(),
                      " ".join(soa_text_parts)))
    

However, this does make me wonder if a basic split() even always works...

OK, your codes is simpler than my codes. I will simplify this codes. For your concern about the split() function, I think you are right, the prerequisite of this function is that the soa RR must exist and not null. I have wrotten down it to comments of this function.

  • Not sure whether it even needs to be a class, or only a few module-level functions; the class does not keep any state at all (so the only purpose seems to be to hide the first two methods)

I also have a few comments about the update_soa() changes;

  • it'll fail if you actually give it a soa with a lower serial (since new_soa won't be set). This btw also suggest that we need to have a unit test that tries that :)
  • the assignment of old to new isn't really necessary in the else-statement; doing the update_soa directly on old_soa should also work
  • both of the above can i think be fixed by merging the nested ifs; something like
    if added_soa is not None and serial_update_check:
      new_soa = added_soa
    else:
      new_soa = update_serial(old_soa)
    
    (names not reflecting actual current code)
    

sorry, jelte, I maybe disagree about this suggestion. :) The rfc 2136 said the update operation should be ignore if the serial number is smaller than the old one. If the new serial number is smaller than the old one, the serial number will be increased according your codes above.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by jelte

  • Owner changed from jelte to haikuo

Replying to haikuo:

The changes done look ok,

sorry, jelte, I maybe disagree about this suggestion. :)

That is allowed :)

The rfc 2136 said the update operation should be ignore if the serial number is smaller than the old one. If the new serial number is smaller than the old one, the serial number will be increased according your codes above.

Unless you are looking at a different section than I am, or I am misreading it, the RFC says to ignore the specific SOA update (not the entire update). It also says in a different place that if the SOA is not updated in the entire update, it should be autoincremented.

To put it another way, the way I read it is that the SOA must be auto-incremented, except if the update contains a SOA RR with a higher serial.

If the serial is lower, it is ignored, and if it is ignored, the SOA would not be updated at all, in which case it would be a violation of the 'must autoincrement if not done by update'.

And even if that is not true, the current code would result in SERVFAIL if you give it a lower serial (since it will try to use new_soa which has not been set to anything) :)

comment:13 in reply to: ↑ 12 Changed 8 years ago by haikuo

Replying to jelte:

Replying to haikuo:

The changes done look ok,

sorry, jelte, I maybe disagree about this suggestion. :)

That is allowed :)

The rfc 2136 said the update operation should be ignore if the serial number is smaller than the old one. If the new serial number is smaller than the old one, the serial number will be increased according your codes above.

Unless you are looking at a different section than I am, or I am misreading it, the RFC says to ignore the specific SOA update (not the entire update). It also says in a different place that if the SOA is not updated in the entire update, it should be autoincremented.

To put it another way, the way I read it is that the SOA must be auto-incremented, except if the update contains a SOA RR with a higher serial.

If the serial is lower, it is ignored, and if it is ignored, the SOA would not be updated at all, in which case it would be a violation of the 'must autoincrement if not done by update'.

And even if that is not true, the current code would result in SERVFAIL if you give it a lower serial (since it will try to use new_soa which has not been set to anything) :)

jelte, the RFC 2136 section 3.6 said each update operation should increase the soa number automatically, but it does't say the soa number should increase if the operation failed.
I tested it using bind9 for this situation above, it looks like that the SOA number was not increased when the update operation failed.
so I need your feedback on how we do in this situation.

comment:14 Changed 8 years ago by jelte

AFAIK, BIND9 takes option 2 from section 3.6, i.e. always update the SERIAL, except in two cases
1) when it is already increased by the update packet
2) if there are no changes at all (except of course the auto-updated serial)

It is assumed that if the update operation failed, the soa is not increased (the entire update is dropped), so I do not consider that a special case.

The most important thing is that it may never happen that zone data changes without the serial being updated. If the update packet contains a SOA with a lower serial then that record is supposed to be ignored, and then the existing soa must be auto-incremented. (from my understanding of the RFC).

I think we should also do option 2, but not necessarily in the update soa code itself; I think we can drop the entire diff if at the end there are no changes at all except the auto-incremented soa.

comment:15 Changed 8 years ago by haikuo

  • Owner changed from haikuo to jelte

I updated my codes, please review it. :)

comment:16 Changed 8 years ago by jelte

  • Owner changed from jelte to haikuo

That looks almost ready, I have one final suggestion;

if the result of find() on line 731 is not ZoneFinder?.SUCCESS, there will be an exception, this is not a really big problem (it will be caught, and a SERVFAIL will be returned), but it is probably nicer for the administrator to check it here, and log an error.

So I suggest to add the code

# We may implement recovering from missing SOA data at some point, but for now servfail on such a broken state
if result != ZoneFinder.SUCCESS:
    raise UpdateError("Error finding SOA record in datasource.", self.__zname, self.__zclass, Rcode.SERVFAIL())

right below the find there (this exception should cause a nicer error to be logged)

comment:17 Changed 8 years ago by haikuo

  • Owner changed from haikuo to jelte

done, I polished it. I also append comments for DDNS_SOA. Make sure every SOAs are not none before invoke function from DDNS_SOA.

comment:18 Changed 7 years ago by jelte

  • Owner changed from jelte to haikuo

OK, looks ready for merge, do you want to do it or shall I? (i suspect there will be a few conflicts now)

comment:19 Changed 7 years ago by jinmei

Why is this ticket still open? Can't we close it?

comment:20 Changed 7 years ago by haikuo

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

done, close the ticket now.

Note: See TracTickets for help on using tickets.