Opened 9 years ago

Closed 9 years ago

#497 closed enhancement (complete)

CNAME processing / CNAME loop detection (reception)

Reported by: shane Owned by: jelte
Priority: medium Milestone: R-Team-Sprint-20110222
Component: resolver Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: 3.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

CNAME processing / CNAME loop detection

  • storing CNAMEs encountered
  • check count of CNAMEs (BIND 9 uses 16)

Subtickets

Change History (7)

comment:1 Changed 9 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:2 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

As part of this ticket, I did a few things more to make things Better:

I moved the response_classifier.[cc|h] to lib/resolve.

I also changed the API a bit; it now takes a const Message& instead of a MessagePtr?, and it takes a (non-const) reference to a Name and an unsigned int, to store the target of a possible cname chain (so that it does not have to do the cname-following dance twice).

With that there, our handleRecursiveAnswer (still in asiolink), now uses the ResponseClassifier? instead of figuring things out itself.

Another addition I did was to lib/dns/message.[cc|h]; I added a clearSection(Message::Section) method, that simply removes all RRs from the given section (used in one of the new helper functions in lib/resolve/resolve.cc, in this case to clear an answer we were building when we find a problem).

Having these changes also allowed me to make the 'client timeout' behaviour as really intended (send back a servfail, but don't stop resolving), so I updated that too.

Ready for review. Branch name trac497, parent commit 1e5b246a41dbfee1fdeb672b07b4e73ee1b94353, final commit 4d29fda48f7304d2c5bfbc246326850ca5970b0f

Proposed changelog(s):

[func] jelte
The resolver now handles CNAMEs, it will follow them, and include them in the answer. The maximum length of CNAME chains that is supported is 16.

[func] jelte
The resolver now sends back a SERVFAIL when there is a client timeout (timeout_client config setting), but it will not stop resolving (until there is a lookup timeout).

comment:3 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:4 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

src/lib/asiolink/asiolink.cc
Should perhaps add a comment to the declaration of cname_count_ in RunningQuery.

handleRecursiveAnswer(): in the "switch" statement, would not a "default" be better than explicitly listing the remaining codes responseClassifier could return? (There is always a chance that ResponseClassifier is altered to extend the number of codes returned - in which case, we probably don't want to abort with a fatal error (as the comment suggests) if we exit the switch.)

Comment: I suggest that comments/suggestions in the comments be highlighted with a "TODO:" or a
"XXX:" - programming environments highlight these, and they are easy to search for. Else we run the risk of such comments being forgotten.

setZoneServersToRoot(): I'm not sure how BOOST_FOREACH could improve over the simple loop, other than to pre-increment the iterator instead of post-incrementing it. (Means the iterator code does not have to save a copy of the iterator before it was incremented - although it is quite possible that the optimiser has optimised that code away.)

src/lib/asiolink/tests/asiolink_unittest.cc
forwardClientTimeout(): when creating a RecursiveQuery, the number of retries has been increased to 4, but the comment above it still refers to three retries.

src/lib/resolve/resolve.h
Comment: copySection really seems to belong to the isc::dns::Message class. In fact, it does occur to me that if we were to make the section numbers bit masks, then we could rename the method copySections() and do all the copying in one call by passing in the logical OR of the sections we wish to copy.

Comment: would "copyResponseMessage()" be a better name than "copyAnswerMessage()", as the fact that there is an "answer" section may make the name slightly confusing.

src/lib/resolve/response_classifier.h
OK, although the "TODO" about code being not being used in this form can be removed as the predicted changes have now been made.

Comment: descriptions of new parameters not in same style as the other parameter descriptions.

ChangeLog Entries
These are fine.

comment:5 in reply to: ↑ 4 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Replying to stephen:

src/lib/asiolink/asiolink.cc
Should perhaps add a comment to the declaration of cname_count_ in RunningQuery.

ack, done

handleRecursiveAnswer(): in the "switch" statement, would not a "default" be better than explicitly listing the remaining codes responseClassifier could return? (There is always a chance that ResponseClassifier is altered to extend the number of codes returned - in which case, we probably don't want to abort with a fatal error (as the comment suggests) if we exit the switch.)

I prefer this construction; now if we forget a statement, the compiler should error (as it would if we extend the list of return codes, in which case i want it added here as well), instead of defaulting to some behaviour. The 'fatal' below would trigger if we forget a return statement in the cases.

Comment: I suggest that comments/suggestions in the comments be highlighted with a "TODO:" or a
"XXX:" - programming environments highlight these, and they are easy to search for. Else we run the risk of such comments being forgotten.

setZoneServersToRoot(): I'm not sure how BOOST_FOREACH could improve over the simple loop, other than to pre-increment the iterator instead of post-incrementing it. (Means the iterator code does not have to save a copy of the iterator before it was incremented - although it is quite possible that the optimiser has optimised that code away.)

Removed the comment. I expect this method to go away in the near future anyway.

src/lib/asiolink/tests/asiolink_unittest.cc
forwardClientTimeout(): when creating a RecursiveQuery, the number of retries has been increased to 4, but the comment above it still refers to three retries.

ack, changed to 4 :)

src/lib/resolve/resolve.h
Comment: copySection really seems to belong to the isc::dns::Message class. In fact, it does occur to me that if we were to make the section numbers bit masks, then we could rename the method copySections() and do all the copying in one call by passing in the logical OR of the sections we wish to copy.

Heh, I think i actually removed a different comment like 'should this be in lib/dns?'

Moved to Message class (and slightly modified it, and added specific tests)

I did not do the mask thing. Not sure if we should rely on Sections being, er, 'maskable', like that. (we can suggest it though, i just didn't do it now)

Comment: would "copyResponseMessage()" be a better name than "copyAnswerMessage()", as the fact that there is an "answer" section may make the name slightly confusing.

ack, cheanged

src/lib/resolve/response_classifier.h
OK, although the "TODO" about code being not being used in this form can be removed as the predicted changes have now been made.

removed

Comment: descriptions of new parameters not in same style as the other parameter descriptions.

changed

ChangeLog Entries
These are fine.

I also added doxygen values for the things Jeremy pointed out in his emails yesterday. (most were related, and while i was at it i added something for the nsas/zoneentry as well)

changes in 81c35ef7e5e01da333654443045de3dd2663482c

comment:6 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Just some minor issues:

src/lib/asiolink/asiolink.cc
The iterator in setZoneServersToRoot() should be pre-incremented, not post-incremented.

src/lib/dns/message.h
Comment: being pedantic, strictly speaking "copySection()" is in fact "appendSection()" - the target section is not replaced, it is appended to.

src/lib/dns/message.cc
copySection(): Still has debug messages to stdout in the code.

Once the changes have been made, please go and merge - I don't need to see it again.

comment:7 Changed 9 years ago by jelte

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

Thanks, merged (git af0e5cd93bebb27cb5c4457f7759d12c8bf953a6), closing ticket

Note: See TracTickets for help on using tickets.