Opened 9 years ago

Closed 9 years ago

#415 closed task (fixed)

new query logic for in memory data source (1)

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: y2 12 month milestone
Component: b10-auth Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 0.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This is a first small step of revised query logic for in memory data source.

From the design memo, in this step we "look up the zone table (which fail) and simply return SERVFAIL."

Subtickets

Change History (8)

comment:1 Changed 9 years ago by jinmei

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

branches/trac415 is ready for review.

The amount code to be reviewed is small and (with document) should be trivial.

It's currently not used from b10-auth so it doesn't break the server behavior.

comment:2 Changed 9 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:3 follow-ups: Changed 9 years ago by jelte

  • Owner changed from jelte to jinmei

I don't have an overview of the entire design and where it fits in (and comment on that would be out of scope for this ticket anyway), so i'll keep it localized;

I only have a few questions/comments, nothing really source code-related (that looks ok)

  • This introduces a second Query class, under a different namespace. Is it supposed to replace the one under datasrc::? I know that that is what namespaces are for, but we tend to use 'using namespace' (which itself i don't really like anyway, btw), and the potential confusion does worry me a bit.
  • note: If I read this correctly, the Rcode that would in the end be set by this code for data that is not found changes from current behaviour (right now it returns REFUSED, not SERVFAIL). I prefer this new one (and do REFUSED because of ACL, not data presence) :)
  • One of the example for future changes in the comments say that we may consider returning the Rcode instead of setting it in the provided response message. If the process() function is going to modify the response by adding results anyway, i think we should also set the Rcode in it. We can then still decide to also return it (or a different simple value that represents success/error)

As said, I think the rest looks ok.

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

Replying to jelte:

  • note: If I read this correctly, the Rcode that would in the end be set by this code for data that is not found changes from current behaviour (right now it returns REFUSED, not SERVFAIL). I prefer this new one (and do REFUSED because of ACL, not data presence) :)

But REFUSED is what you get from BIND 9.

Of course, BIND 9 *can* do recursion, and it's the allow-query-cache and allow-recursion ACLs that prevent it from doing so, so the REFUSED *is* happening because of an ACL, not data presence. But we're aiming for compatibility with BIND 9 here, and auth-only name servers send REFUSED when you ask for a nonexistent zone.

(REFUSED may be a better choice than SERVFAIL anyway because a resolver that gets SERVFAIL will keep trying other name servers in the NS RRset, whereas with REFUSED it could give up immediately.)

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

Replying to each:

But REFUSED is what you get from BIND 9.

Of course, BIND 9 *can* do recursion, and it's the allow-query-cache and allow-recursion ACLs that prevent it from doing so, so the REFUSED *is* happening because of an ACL, not data presence. But we're aiming for compatibility with BIND 9 here, and auth-only name servers send REFUSED when you ask for a nonexistent zone.

interesting, I did not know that. I know at least one competitor that returns SERVFAIL ;)

(REFUSED may be a better choice than SERVFAIL anyway because a resolver that gets SERVFAIL will keep trying other name servers in the NS RRset, whereas with REFUSED it could give up immediately.)

If that is so, it sounds like a feature, not a bug. After all, there is a reasonable chance that the other servers in the NS set are not lame...

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

Replying to jelte:

I don't have an overview of the entire design and where it fits in (and comment on that would be out of scope for this ticket anyway), so i'll keep it localized;

A higher level idea is documented here:
http://bind10.isc.org/wiki/AuthQueryLogic
(although I'm not sure if this give you an overview you mean here)

I only have a few questions/comments, nothing really source code-related (that looks ok)

  • This introduces a second Query class, under a different namespace. Is it supposed to replace the one under datasrc::? I know that that is what namespaces are for, but we tend to use 'using namespace' (which itself i don't really like anyway, btw), and the potential confusion does worry me a bit.

Not necessarily "replace", but we should eventually merge the two modules, at which point there will be only one "Query" class. I was actually aware of the duplicate name and possible confusion, and considered a different name "AuthQuery?" just to avoid the ambiguity. But I chose not to do so because the name sounds redundant in some sense and the ambiguity will be temporary anyway.

  • note: If I read this correctly, the Rcode that would in the end be set by this code for data that is not found changes from current behaviour (right now it returns REFUSED, not SERVFAIL). I prefer this new one (and do REFUSED because of ACL, not data presence) :)

Your understanding is correct. I know it's different from the typical response of BIND 9 in this configuration and BIND 10's libdatasrc. If compatibility with BIND 9 in this case is important, I don't mind changing the behavior, but I guess this will be a broader question and we should discuss it at the bind10-dev list.

  • One of the example for future changes in the comments say that we may consider returning the Rcode instead of setting it in the provided response message. If the process() function is going to modify the response by adding results anyway, i think we should also set the Rcode in it. We can then still decide to also return it (or a different simple value that represents success/error)

Okay, I've updated the doxygen comment accordingly.

I've also updated the comments for the other two points, and I'm going to merge this version to trunk. If some of the questions require a further change, we'll handle it as a separate task.

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

Replying to jelte:

(REFUSED may be a better choice than SERVFAIL anyway because a resolver that gets SERVFAIL will keep trying other name servers in the NS RRset, whereas with REFUSED it could give up immediately.)

If that is so, it sounds like a feature, not a bug. After all, there is a reasonable chance that the other servers in the NS set are not lame...

As far as I understand it at least BIND 9 doesn't treat REFUSED as a fatal error. It will simply keep trying other servers. I suspect other widely used recursive servers behave that way, too. In fact, this type of lame is not uncommon (it can easily happen due to parent-child mismatch, for example), and if it triggered a perpanet failure we should have seen a lot of complaints.

comment:8 Changed 9 years ago by jinmei

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

merged to trunk, closing ticket.

Note: See TracTickets for help on using tickets.