Opened 8 years ago

Closed 8 years ago

#1299 closed task (fixed)

compare SOA serial in xfrin

Reported by: jinmei Owned by: jinmei
Priority: very high Milestone: Sprint-20111220
Component: xfrin Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: AXFR-in
Estimated Difficulty: 3 Add Hours to Ticket:
Total Hours: 10.63 Internal?: no

Description

This is another follow-up task for #1261. This one depends on #1278.

We need to complete this part of xfrin.

class XfrinInitialSOA(XfrinState):
    def handle_rr(self, conn, rr):
[...]
        # FIXME: we need to check the serial is actually greater than ours.
        # To do so, however, we need to implement serial number arithmetic.
        # Although it wouldn't be a big task, we'll leave it for a separate
        # task for now.  (Always performing xfr could be inefficient, but
        # shouldn't do any harm otherwise)

We also need to complete this part (although technically it's not a
follow-up for #1261):

    def _check_soa_serial(self):
[...]
        # TODO, need select soa record from data source then compare the two
        # serial, current just return OK, since this function hasn't been used
        # now.
        return XFRIN_OK

Subtickets

Change History (19)

comment:1 follow-up: Changed 8 years ago by jreed

This should fix long delay that leads to "receive data from socket time out." During this delay, manual Xfrin retransfer does not work (due to locked "zone xfrin is in progress").

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

Replying to jreed:

This should fix long delay that leads to "receive data from socket time out." During this delay, manual Xfrin retransfer does not work (due to locked "zone xfrin is in progress").

Correction: in this situation, manual retransfer doesn't really work anyway,
because it will also fail. Actually, we should probably use AXFR when
the 'retransfer' command is issued (BIND 9's 'rndc retransfer' forces
AXFR).

comment:3 Changed 8 years ago by jelte

  • Add Hours to Ticket changed from 0 to 3

comment:4 Changed 8 years ago by jelte

  • Add Hours to Ticket 3 deleted
  • Estimated Difficulty changed from 0 to 3

comment:5 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to New Tasks

comment:6 Changed 8 years ago by jinmei

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:7 Changed 8 years ago by jinmei

  • Summary changed from ccompare SOA serial in xfrin to compare SOA serial in xfrin

comment:8 Changed 8 years ago by jelte

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

comment:9 Changed 8 years ago by shane

  • Feature Depending on Ticket set to AXFR-in

comment:10 Changed 8 years ago by jinmei

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

comment:11 Changed 8 years ago by jinmei

trac1299 is ready for review.

(As always?) The branch resulted in a bit bigger than I originally
expected. If the branch is deemed to big to review as a whole, the
following categorization might help:

  • The first two commits are separate initial cleanups and can be reviewed separately.
  • The third one (4e636c3) is an independent bug fix. But the rest of the code depends on it, so I included it in this branch. This is a completely independent change and can be reviewed separately.
  • Commits from 8376919 to 50fdb09 (inclusive) are one of the two main subjects of this task and handles SOA serial comparison for SOA query responses. This part was relatively big to handle various validations on the responses (not really the subject of this task but the existing code was just way too native about bogus responses.)
  • The last commit (b3c9fff) was the second part of the main subject of this task and handles SOA serial comparison for IXFR responses.

Proposed changelog entry is as follows:

337.?	[bug]		jinmei
	b10-xfrin didn't check SOA serials of SOA and IXFR responses,
	which resulted in unnecessary transfer or unexpected IXFR
	timeouts (these issues were not overlooked but deferred to be
	fixed until #1278 was completed).  Validation on responses to SOA
	queries were tighten, too.
	(Trac #1299, git TBD)

comment:12 Changed 8 years ago by jinmei

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

comment:13 Changed 8 years ago by jelte

  • Owner changed from UnAssigned to jelte

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

  • Owner changed from jelte to jinmei

I fixed a couple of typos, see commit TODO

xfrin.py.in:

in parse_soa_response:

            raise XfrinProtocolError('Invalid number of questions to ' +
                                     'SOA query (' + str(n_question) + ')')

While correct, I fear that this message might prove unuseful; we are talking about a response where, so I suggest making it something like "Invalid number of records in question section of response to SOA query", or something.

Similar issue for the next exception 'Questions mismatch to ')

(the rest seems fine in this regard, although the 'at all' in 'SOA query resulted in no SOA at all' might only make sense in the context of the other possible errors :) I'd suggest 'No SOA record found in response to SOA query')

<log-level-bikeshed> should XFRIN_ZONE_SERIAL_AHEAD be a WARN instead of INFO? (no strong opinion, just wondering, it is something that should probably be looked in to) </log-level-bikeshed>

For the rest it looks good

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

Replying to jelte:

Thanks for the review.

I fixed a couple of typos, see commit TODO

Thanks, they look good.

xfrin.py.in:

in __parse_soa_response:

            raise XfrinProtocolError('Invalid number of questions to ' +
                                     'SOA query (' + str(n_question) + ')')

While correct, I fear that this message might prove unuseful; we are talking about a response where, so I suggest making it something like "Invalid number of records in question section of response to SOA query", or something.

Similar issue for the next exception 'Questions mismatch to ')

I see the point. I've updated them in the latest commit. Hopefully
they are better.

(the rest seems fine in this regard, although the 'at all' in 'SOA query resulted in no SOA at all' might only make sense in the context of the other possible errors :) I'd suggest 'No SOA record found in response to SOA query')

Ack. Applied in the same commit.

<log-level-bikeshed> should XFRIN_ZONE_SERIAL_AHEAD be a WARN instead of INFO? (no strong opinion, just wondering, it is something that should probably be looked in to) </log-level-bikeshed>

In general, I personally believe WARN/ERROR is too verbose for events
that are triggered outside of the system. So my general opinion
applied here.

It's also consistent with BIND 9's policy:

                        dns_zone_log(zone, ISC_LOG_INFO, "serial number (%u) "
                                     "received from master %s < ours (%u)",
                                     soa.serial, master, oldserial);

I've been aware that the policy on the verbosity level is not very
consistent among developers. Maybe a topic for the biweekly call?

comment:16 Changed 8 years ago by jinmei

  • Owner changed from jinmei to jelte

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

  • Owner changed from jelte to jinmei

Replying to jinmei:

xfrin.py.in:

While correct, I fear that this message might prove unuseful; we are talking about a response where, so I suggest making it something like "Invalid number of records in question section of response to SOA query", or something.

I see the point. I've updated them in the latest commit. Hopefully
they are better.

ack :)

<log-level-bikeshed> should XFRIN_ZONE_SERIAL_AHEAD be a WARN instead of INFO? (no strong opinion, just wondering, it is something that should probably be looked in to) </log-level-bikeshed>

In general, I personally believe WARN/ERROR is too verbose for events
that are triggered outside of the system. So my general opinion
applied here.

It's also consistent with BIND 9's policy:

                        dns_zone_log(zone, ISC_LOG_INFO, "serial number (%u) "
                                     "received from master %s < ours (%u)",
                                     soa.serial, master, oldserial);

I've been aware that the policy on the verbosity level is not very
consistent among developers. Maybe a topic for the biweekly call?

I seem to remember that we've had such a discussion before, I'm not opposed but I'm afraid that it will again result in 'we need some policy and look at it on a case-by-case basis', but we can certainly propose it as an agenda item. Regarding this choice, if it is what bind9 did I'm fine with it (and your reasoning is, er, reasonable too).

So please merge :)

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

Replying to jelte:

I've been aware that the policy on the verbosity level is not very
consistent among developers. Maybe a topic for the biweekly call?

I seem to remember that we've had such a discussion before, I'm not opposed but I'm afraid that it will again result in 'we need some policy and look at it on a case-by-case basis', but we can certainly propose it as an agenda item. Regarding this choice, if it is what bind9 did I'm fine with it (and your reasoning is, er, reasonable too).

Pushed it in my local memo on possible biweekly topics.

So please merge :)

Merge done, closing ticket.

Thanks,

comment:19 Changed 8 years ago by jinmei

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 10.63
Note: See TracTickets for help on using tickets.