Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1390 closed task (complete)

update b10-auth to pass IXFR-request to xfrout

Reported by: jinmei Owned by: jelte
Priority: medium Milestone: Sprint-20111122
Component: b10-auth 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: 4.75 Internal?: no

Description

Theis is a missing feature for the support for IXFR-out. This is
probably breaking the law, but hoping we might still be able to complete
other IXFR-out tasks, I'm going to push this to the current sprint.

If anyone feels it's inappropriate, please pull it back.

Subtickets

Change History (11)

comment:1 Changed 8 years ago by jelte

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

I don't think it is inappropriate, and I even think it'll help getting #1371 and #1372 done (since you can then directly play with what you are building)

So I've did this; change the name of processAxfr to processXfr in auth, and not return NOTIMPL on IXFR requests.

The changes on the xfrout daemon are minimal; it does one extra check (to see if there is a SOA in the request if it is an IXFR), and I've renamed a few log identifiers (they had AXFR in their name, but are valid for both AXFR and IXFR). It now responds exactly as if it was a AXFR request (since the handling is to be done in #1371/2).

comment:2 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:3 Changed 8 years ago by vorner

  • Owner changed from vorner to jelte

comment:4 follow-up: Changed 8 years ago by vorner

Hello

I made a small whitespace change, there were spaces at the end of lines.

As for the comments to the branch:

  • In the test checking the successful scenario, should it check that the auth provides no answer at all?
  • There are two „# TODO: Log?“. Maybe we should decide and maybe really log.
  • Indentation here (the lower line doesn't start at just after the opening parenthesis):
    -            sendAnswer = impl_->processAxfrQuery(io_message, message, buffer,
    +            sendAnswer = impl_->processXfrQuery(io_message, message, buffer,
                                                      tsig_context);
    
-            makeErrorMessage(message, buffer, Rcode::NOTIMP(), tsig_context);
+            sendAnswer = impl_->processXfrQuery(io_message, message, buffer,
+                                                 tsig_context);

This one many times:

+    UnitTestUtil::createRequestMessage(request_message, opcode, default_qid,
+                         Name("example.com"), RRClass::IN(), RRType::IXFR());

This changes the external behaviour, so I think this would need a changelog entry.

Thank you

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

  • Owner changed from jelte to vorner

Replying to vorner:

Hello

I made a small whitespace change, there were spaces at the end of lines.

thanks

As for the comments to the branch:

  • In the test checking the successful scenario, should it check that the auth provides no answer at all?

Ack. Also fixed the inline comments that were still talking about
AXFR. And, for completeness, added the same check to the similar AXFR
test.

  • There are two „# TODO: Log?“. Maybe we should decide and maybe

really log.

ack, three even :) added log messages. After a short bit of discussion
on jabber, I've set these to debug level (otherwise externals could
cause log spam), except for the one that should never even reach this
part of the code. I also changed the acl reject/drop from info to
debug, for the same reasoning.

  • Indentation here (the lower line doesn't start at just after the opening parenthesis):

snip

This one many times:

snip

Fixed.

This changes the external behaviour, so I think this would need a changelog entry.

Hmm, yes, I don't know if we were aiming for one Big Changelog
message, but let's go for:

[func] jelte
The b10-auth will no longer reply to IXFR requests with a NOTIMPL, but
pass the IXFR request on to b10-xfrout (if running). Currently,
pending #1371 and #1372, xfrout will not correctly respond to IXFR
requests, but it will answer as if it was an AXFR.

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

  • Owner changed from vorner to jelte

Hello

Replying to jelte:

  • There are two „# TODO: Log?“. Maybe we should decide and maybe

really log.

ack, three even :) added log messages. After a short bit of discussion
on jabber, I've set these to debug level (otherwise externals could
cause log spam), except for the one that should never even reach this
part of the code. I also changed the acl reject/drop from info to
debug, for the same reasoning.

Hmm, looks like there'll be a collision with #1372, as it touches the area as well. But this branch is OK in itself.

Hmm, yes, I don't know if we were aiming for one Big Changelog
message, but let's go for:

[func] jelte
The b10-auth will no longer reply to IXFR requests with a NOTIMPL, but
pass the IXFR request on to b10-xfrout (if running). Currently,
pending #1371 and #1372, xfrout will not correctly respond to IXFR
requests, but it will answer as if it was an AXFR.

I'm not sure if it will be true on the release, the #1371 and #1382 are almost ready to merge as well. So, maybe the note about response to IXFR might be unneeded.

But the branch is ready to merge.

Thank you

comment:7 Changed 8 years ago by vorner

  • Total Hours changed from 0 to 0.75

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

Replying to vorner:

I'm not sure if it will be true on the release, the #1371 and #1382 are almost ready to merge as well. So, maybe the note about response to IXFR might be unneeded.

But the branch is ready to merge.

The merge with master (already containing #1371 and #1372) would cause
non trivial conflicts, so I've made suggested resolution and pushed it
on the branch. Could you check that and if it's okay then merge it
back to master?

The most important fix would be that the check against empty
authority section is now in __ixfr_setup() so it was removed
from _parse_query_message(). create_request_data() in the test
was adjusted accordingly, and a test case in the original #1390
on this point was removed (the scenario is tested in
test_xfrout_ixfr_setup(), although it actually checks the case
where the authority section has an SOA but it doesn't match the
zone name).

I've also made some cleanups regarding logging: removed now-unused
messages, fixing typo, etc.

Also, the changelog for this entry should be changed now #1371
and #1372 have been merged.

comment:9 Changed 8 years ago by jinmei

  • Total Hours changed from 0.75 to 1.75

comment:10 Changed 8 years ago by jelte

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

Ok, thanks, merged, closing ticket.

comment:11 Changed 8 years ago by jelte

  • Total Hours changed from 1.75 to 4.75
Note: See TracTickets for help on using tickets.