Opened 8 years ago

Closed 8 years ago

#1262 closed task (complete)

AXFR-style IXFR-in protocol handling

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

Description

This is a subtask of #1212 and depends on #1258, #1259, and #1260.
But it should be possible to start working on it without waiting for
them to fully complete.

It's also related to #1261, and if it's developed in parallel with
#1261, the two developers need to coordinate.

The implementation of this task can simply be to forward the job to
the existing AXFR code with some minor tweak in the existing code that
handles AXFR (e.g., changing _handle_xfrin_response() so that it can
both handle the case of receiving a message and the case of forwarded
RR sequences).

Subtickets

Change History (11)

comment:1 Changed 8 years ago by jinmei

I've pushed a mostly complete (feature wise) snapshot of #1261 at branch
trac1261. There may be some design level dicussions in the implementation
that can require substantial changes, but if someone is looking for a
new task related to IXFR, I believe this task can be started on top of
the current snapshot of trac1261.

What would have to be done is to update _handle_xfrin_responses()
so that after calling handle_rr() if the new state is XfrinAXFR
switch to the existing sqlite3-specific AXFR code (which will have
to be adjusted a bit to support the both cases). Of course,
this is intended to be a short term workaround, and the AXFR
part should also be integrated to the new code. (Or if it's
deemed to be easy at this point, we may be able to complete that
part within this sprint).

comment:2 Changed 8 years ago by vorner

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

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

Assuming this is relatively an easy and small task, I'd suggest
adding one small feature: deciding AXFR vs IXFR.

For example, BIND 9 decides it as follows:

  1. If the zone doesn't have a corresponding DB, use AXFR.
  2. Otherwise, if it's a result of 'rndc retransfer', use AXFR.
  3. Otherwise, if it's known IXFR hasn't worked for the zone in previous attempts, use AXFR.
  4. Otherwise, decide IXFR vs AXFR based on the server configuration (per server and per zone basis) (It basically defaults to IXFR).

Right now, we partially do 4. We may also emulate 2, if it's a result
of the 'retransfer' command via bindctl. 1 would be necessary for the
very first transfer (unless some copy of the zone is manually
installed).

comment:4 in reply to: ↑ 3 Changed 8 years ago by vorner

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

Hello

Replying to jinmei:

What would have to be done is to update _handle_xfrin_responses()
so that after calling handle_rr() if the new state is XfrinAXFR
switch to the existing sqlite3-specific AXFR code (which will have
to be adjusted a bit to support the both cases). Of course,
this is intended to be a short term workaround, and the AXFR
part should also be integrated to the new code. (Or if it's
deemed to be easy at this point, we may be able to complete that
part within this sprint).

I went the new path, because it seemed less work than trying to persuade the generator to steal the connection in some way. It indeed turned out to be quite easy (after understanding the surrounding code, the new one is nice and easy to read, but the old one needs a cleanup).

However, I did not modify the original AXFR code to use this new one. I'd like to merge this as soon as possible so we see if IXFR-in really works and have time to fix problems before release. If there's time, I'm not against doing the change in a follow-up ticket (or keeping this open after the merge), of course.

Replying to jinmei:

Assuming this is relatively an easy and small task, I'd suggest
adding one small feature: deciding AXFR vs IXFR.

I'd postpone this to a follow-up ticket for the same reasons as well. That might be nice feature, but better have something than not having time to merge a whole larger branch.

Thanks

comment:5 Changed 8 years ago by jinmei

  • Owner changed from UnAssigned to jinmei

comment:6 follow-up: Changed 8 years ago by jinmei

I have some points to discuss that may be debatable. To save time,
however, I made suggested changes on the branch. If you agree with
the sense of the change and the changes themselves, I have no further
issue. As soon as #1261 is merged, this branch can be merged too.

If you don't agree or do want to make more changes, please respond
(the committed changes were just a suggestion, not a requiremnet).

Postponing other things is okay for me.

Specific comments and suggestions below:

XfrinAXFR class

I'd use conn._diff instead of an attribute of XfrinAXFR class for the
Diff object of AXFR. One reason is that XfrinState? classes are
expected to be "stateless" (so they wouldn't have a mutable member
attribute). More important reason is that I'd delay the commit until
XfrinAXFREnd.finish_message() (which is the topic of the next
paragraph), and to keep the diff over multiple states it would be
easier to hold it in XfrinConn? (as indicated in your comment).

Now, as for delaying commit. In case of AXFR, I believe we should
perform some more stricter checks on the received zone, e.g., whether
the zone has an NS RR before actually publishing the zone (BIND 9
behaves that way). We could do this at the end of the XfrinAXFR
state, but if we do this check I'd also include other checks such
as whether the message doesn't contain a bogus trailing RR.

(Note also that to perform such check we need to ensure all buffered
changes in Diff are really pushed to the data source before actually
committing the transaction. So we'll need some extension to the
Diff object, or we may simply want to use an updater directly for
AXFR. But that should be beyond the scope of this task)

I made my proposed change based on this. It's commit

xfrin_test

I would add some higher level tests, i.e., AXFR-style versions of
TestIXFRSession and TestIXFRSessionWithSQLite3. I added some
suggested tests to the branch.

comment:7 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by vorner

Hello

Replying to jinmei:

I have some points to discuss that may be debatable. To save time,
however, I made suggested changes on the branch. If you agree with
the sense of the change and the changes themselves, I have no further
issue. As soon as #1261 is merged, this branch can be merged too.

The changes look OK, thanks for them.

(Note also that to perform such check we need to ensure all buffered
changes in Diff are really pushed to the data source before actually
committing the transaction. So we'll need some extension to the
Diff object, or we may simply want to use an updater directly for
AXFR. But that should be beyond the scope of this task)

The Diff.apply() does just that, pushes all changes to the DB. So we don't need to extend it.

Also, I used the Diff to make the changes buffered, otherwise we could use the updater directly within IXFR-in as well.

I'm keeping the ticket and I'll try to keep an eye on the other ticket, then I'll merge.

Thanks

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

Replying to vorner:

The Diff.apply() does just that, pushes all changes to the DB. So we don't need to extend it.

Also, I used the Diff to make the changes buffered, otherwise we could use the updater directly within IXFR-in as well.

I'm keeping the ticket and I'll try to keep an eye on the other ticket, then I'll merge.

#1261 has been merged. This can go, too.

I expect you'll see some non trivial conflicts in merge. Be careful
about them.

comment:10 Changed 8 years ago by jinmei

oh, and one more thing. I pushed one small doc update to the branch. please pull it first.

comment:11 Changed 8 years ago by vorner

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

Thanks, closing.

Note: See TracTickets for help on using tickets.