Opened 8 years ago

Closed 8 years ago

#1611 closed task (fixed)

ZoneFinder::find refactoring: separate normal result codes and special case flags

Reported by: jinmei Owned by: jinmei
Priority: medium Milestone: Sprint-20120207
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: 0
Total Hours: 2.5 Internal?: no

Description

See #1431, especially http://bind10.isc.org/ticket/1431#comment:17

This is a follow up task of that topic:

  • remove WILDCARD_xxx from the result codes
  • update the data base finder implementation accordingly: have it use the flags for wildcard and NSEC (and eventually NSEC3) cases
  • update database test accordingly
  • update python wrapper and its applications and tests accordingly
  • update documentation

This is basically a straightforward refactoring (actually I've mostly
done as of this writing), but many parts of the code will have to be
modified.

This task is not absolutely necessary for completing NSEC3 in in
memory, but I hope we can complete it sooner than later (if not in
this coming sprint, in the next one).

Subtickets

Change History (11)

comment:1 Changed 8 years ago by jinmei

(I know this is not in the current sprint, but this one is essentially
an immediate followup of #1431, and according to the review
discussions for #1431 it seemed quite likely that this ticket would be
included in the next sprint anyway - besides I didn't find anything
I can quickly complete in the remaining tickets of the current sprint)

trac1611 is ready for review.

As already noted, this is basically straightforward interface update,
but many parts of the code were affected and the size of diff is quite
large.

This is a backward incompatible change to a public API, so I think we
should provide a changelog entry. This is the proposed one:

365.?	[func]*		jinmei
	libdatasrc: the interface of ZoneFinder() was changed: WILDCARD
	related result codes were deprecated and removed, and the
	corresponding information is now provided via a separate accessor
	method on FindResult.  Other separate FindResult methods will
	also tell the caller whether the zone is signed with NSEC or NSEC3
	(when necessary and applicable).
	(Trac #1611, git TBD)

comment:2 Changed 8 years ago by jinmei

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

comment:3 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 4

comment:4 Changed 8 years ago by jinmei

  • Milestone changed from Year 3 Task Backlog to Sprint-20120207

(moving to the current sprint as discussed on jabber)

comment:5 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

First, is it OK that we're kind of inconsistent with python? When we had the previous version of FindResult?, it acted mostly as a tuple. But now the C++ version has methods, while the python one has item in the tuple. But it is probably the easiest way to implement it.

Is the removal of ZoneFinder:: from the front of FindResult? returns in database.cc an unrelated cleanup or does it have any meaning in the scope of the ticket?

Is this missing the NSEC3 flag?

// For wildcard case with DNSSEC required, the caller would need to know
// whether it's NSEC or NSEC3 signed.  So we need to do an additional
// search here, even though the NSEC RR may not be returned.
if (wild && (options & FIND_DNSSEC) != 0 &&
    found.second.find(RRType::NSEC()) != found.second.end()) {
    flags = flags | RESULT_NSEC_SIGNED;
}

This is not true in the python part, as it returns a tuple, not an object, so it has no methods:

The returned FindResult object can also provide supplemental\n\
information about the search result via its methods returning a\n\
boolean value. Such information may be useful for the caller if the\n\
caller wants to collect additional DNSSEC proofs based on the search\n\
result.\n\

Why is this change included here? Because it is wrong, the python find_all doesn't have a target parameter (because return parameters are very unpythonish).

 Parameters:\n\
-  name       The domain name to be searched for.\n\
+  target     the successfull result is returned through this\n\
   options    The search options.\n\

This is missing one combination (if there are 3 items, we can as well check comparing all of them):

def test_findresultflags(self):
    self.assertNotEqual(ZoneFinder.RESULT_WILDCARD,
                        ZoneFinder.RESULT_NSEC_SIGNED)
    self.assertNotEqual(ZoneFinder.RESULT_NSEC_SIGNED,
                        ZoneFinder.RESULT_NSEC3_SIGNED)

Should this be checked for equality, so no extra flag is returned (multiple occurrences)

 self.assertEqual(finder.SUCCESS, result)
+self.assertTrue((flags & finder.RESULT_WILDCARD) != 0)
 self.assertEqual("foo.wild.example.com. 3600 IN A 192.0.2.255\n",

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

Replying to vorner:

Thanks for the prompt review.

First, is it OK that we're kind of inconsistent with python? When we had the previous version of FindResult?, it acted mostly as a tuple. But now the C++ version has methods, while the python one has item in the tuple. But it is probably the easiest way to implement it.

Good question. I was aware of the mismatch, but I didn't fully think
about it. Honestly, I don't know what's the best. In some sense
using the tuple instead of a dedicated class is already a non trivial
difference, so this would probably not so special. I also thought
that in dynamic languages like python it wouldn't make much sense to
prevent "abuse" via interface - that's one slight reason I used a bare
integer. Also, in the vast majority cases Python applications
wouldn't care about the optional flags information (they would
basically want to know the simple result of whether a given name/type
exists in the zone, and if it does, what it is in the form of RRset).
On the other hand, I agree that it's generally better to keep these
two interfaces as compatible as possible.

If you have a specific suggestion, I'm happy to hear it. Otherwise,
it's probably okay to leave it open for now (especially for the last
reason above).

Is the removal of ZoneFinder:: from the front of FindResult? returns in database.cc an unrelated cleanup or does it have any meaning in the scope of the ticket?

Sorry for not being clear - it was just an unrelated cleanup. I
noticed they can be omitted in the same name scope (as a class) and it
helped improve brevity.

Is this missing the NSEC3 flag?

// For wildcard case with DNSSEC required, the caller would need to know
// whether it's NSEC or NSEC3 signed.  So we need to do an additional
// search here, even though the NSEC RR may not be returned.
if (wild && (options & FIND_DNSSEC) != 0 &&
    found.second.find(RRType::NSEC()) != found.second.end()) {
    flags = flags | RESULT_NSEC_SIGNED;
}

Not really, but I admit the intent wasn't clear. Right now we are not
sure how we support NSEC3 in the database based backend, and how we
set the NSEC3_SIGNED flag would depend on it. Even for the NSEC case
the additional search is probably not the best way (it's expensive,
and for some half-broken zones it might be possible that the zone is
signed with NSEC but some names miss corresponding NSEC RRs). This
fragment of code is the current workaround to adjust the
implementation to the new interface with keeping compatibility, and I
guess we should revisit it when we support NSEC for the database
backend. I've updated code comment so the intent is (hopefully)
clearer.

This is not true in the python part, as it returns a tuple, not an object, so it has no methods:

The returned FindResult object can also provide supplemental\n\
information about the search result via its methods returning a\n\
boolean value. Such information may be useful for the caller if the\n\
caller wants to collect additional DNSSEC proofs based on the search\n\
result.\n\

Ah, right. I've re-checked the pydoc and found some other cases that
mention FindResult?. I believe I've fixed all cases that had a
mismatch on this point.

Why is this change included here? Because it is wrong, the python find_all doesn't have a target parameter (because return parameters are very unpythonish).

 Parameters:\n\
-  name       The domain name to be searched for.\n\
+  target     the successfull result is returned through this\n\
   options    The search options.\n\

Oops, that was a mistake (I think). Fixed it.

This is missing one combination (if there are 3 items, we can as well check comparing all of them):

def test_findresultflags(self):
    self.assertNotEqual(ZoneFinder.RESULT_WILDCARD,
                        ZoneFinder.RESULT_NSEC_SIGNED)
    self.assertNotEqual(ZoneFinder.RESULT_NSEC_SIGNED,
                        ZoneFinder.RESULT_NSEC3_SIGNED)

Fair enough, done.

Should this be checked for equality, so no extra flag is returned (multiple occurrences)

 self.assertEqual(finder.SUCCESS, result)
+self.assertTrue((flags & finder.RESULT_WILDCARD) != 0)
 self.assertEqual("foo.wild.example.com. 3600 IN A 192.0.2.255\n",

Okay, done.

comment:8 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

First, is it OK that we're kind of inconsistent with python? When we had the previous version of FindResult?, it acted mostly as a tuple. But now the C++ version has methods, while the python one has item in the tuple. But it is probably the easiest way to implement it.

Good question. I was aware of the mismatch, but I didn't fully think
about it. Honestly, I don't know what's the best. In some sense
using the tuple instead of a dedicated class is already a non trivial
difference, so this would probably not so special. I also thought
that in dynamic languages like python it wouldn't make much sense to
prevent "abuse" via interface - that's one slight reason I used a bare
integer. Also, in the vast majority cases Python applications
wouldn't care about the optional flags information (they would
basically want to know the simple result of whether a given name/type
exists in the zone, and if it does, what it is in the form of RRset).
On the other hand, I agree that it's generally better to keep these
two interfaces as compatible as possible.

If you have a specific suggestion, I'm happy to hear it. Otherwise,
it's probably okay to leave it open for now (especially for the last
reason above).

I'm not sure. Having an object with methods as a return value of something to just carry the information is unusual for python. So doing it in completely same way as in C++ is probably a bad idea, it would surprise python programmers.

There could be a hybrid approach ‒ something that'd be Iterable and still had the methods. So one could call it the original way, eg:

code, rrset = finder.find(name, type)

or it could be used as:

result = finder.find(name, type)
code, rrset = result
if result.is_wildcard():
        something()

However, that might be even more surprising and would need quite some code to create the class which is returned.

Therefore I think keeping it the tuple way is probably the way to go, as it is expected from python.

The rest of the code looks OK, so I'd think it is good for merge.

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

Replying to vorner:

However, that might be even more surprising and would need quite some code to create the class which is returned.

Yeah, the consistency about the accessor isXXX() methods doesn't seem
worth the additional complexity.

Therefore I think keeping it the tuple way is probably the way to go, as it is expected from python.

The rest of the code looks OK, so I'd think it is good for merge.

Okay, thanks, merge done. Closing.

comment:11 Changed 8 years ago by jinmei

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