Opened 8 years ago

Closed 8 years ago

#504 closed enhancement (fixed)

CNAME Implementation

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: A-Team-Sprint-20110126
Component: data source 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

Support for CNAME in in-memory data source

Subtickets

Change History (14)

comment:1 Changed 8 years ago by jinmei

Copied from the previous sprint planning page:
Update MemoryZoneImpl::find() and auth::Query::process() to cover CNAME case. Note that it should not create an infinite loop of CNAME chain.

comment:2 Changed 8 years ago by jinmei

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

comment:3 Changed 8 years ago by jinmei

I've decided to divide this task into the MemoryZone? part and auth::Query
part as the former seems to be sufficiently large and these two parts
are actually orthogonal (we can/should test the latter without the former).

I've completed the MemoryZone? part, which is in branch trac504. It's
ready for review.

I'll start working on the auth::Query part while I'm waiting for review
results.

comment:4 Changed 8 years ago by jinmei

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

comment:5 Changed 8 years ago by jinmei

  • Component changed from b10-auth to data source

comment:6 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Two things:

  • Is the MemoryZone? the right place to check for singleton RRs? If we really want to check that, then I guess RRset itself should throw in that case, this is not specific to memory zones.
  • Is the different logic under a zone cut needed? I don't see why it should be different and maybe it would be simpler to just act the same way every time.

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

Replying to vorner:

Two things:

  • Is the MemoryZone? the right place to check for singleton RRs? If we really want to check that, then I guess RRset itself should throw in that case, this is not specific to memory zones.

One difficulty is that we may not always be able to detect violation of singleton in the form of a single RRset. In general, a zone can be given two RRs of the same name and of a single type from different RRsets, so we need to do something within zones anyway.

Whether or not we should do this check at the level of RRset::addRdata() is a different question, and it's probably a good idea. I think it's a matter of separate ticket/task, though.

  • Is the different logic under a zone cut needed? I don't see why it should be different and maybe it would be simpler to just act the same way every time.

Good question. This behavior was copied from BIND 9, and I actually
wondered about the rationale of it. At least I don't know a standard
that specifies this behavior. I'll ask BIND 9 designers/developers about
the intent, but for now I guess it's okay to remove this special case
for simplicity.

comment:9 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

Replying to jinmei:

  • Is the different logic under a zone cut needed? I don't see why it should be different and maybe it would be simpler to just act the same way every time.

Good question. This behavior was copied from BIND 9, and I actually
wondered about the rationale of it. At least I don't know a standard
that specifies this behavior. I'll ask BIND 9 designers/developers about
the intent, but for now I guess it's okay to remove this special case
for simplicity.

This is done. Is there anything I should address in the context of
this branch?

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

  • Owner changed from vorner to jinmei

Replying to jinmei:

Replying to vorner:

  • Is the MemoryZone? the right place to check for singleton RRs? If we really want to check that, then I guess RRset itself should throw in that case, this is not specific to memory zones.

One difficulty is that we may not always be able to detect violation of singleton in the form of a single RRset. In general, a zone can be given two RRs of the same name and of a single type from different RRsets, so we need to do something within zones anyway.

Well, at some point they would be put together and at that point they would throw an exception.

Whether or not we should do this check at the level of RRset::addRdata() is a different question, and it's probably a good idea. I think it's a matter of separate ticket/task, though.

Did you create the ticket or should I?

  • Is the different logic under a zone cut needed? I don't see why it should be different and maybe it would be simpler to just act the same way every time.

Good question. This behavior was copied from BIND 9, and I actually
wondered about the rationale of it. At least I don't know a standard
that specifies this behavior. I'll ask BIND 9 designers/developers about
the intent, but for now I guess it's okay to remove this special case
for simplicity.

Thanks. It seems OK now, so merge, please.

comment:12 Changed 8 years ago by each

About a month ago, there was a discussion on the ISC tech-staff mailing list about CNAME chaining behavior (ISC personnel can review it at https://wiki.isc.org/mhonarc/tech-staff/msg08812.html.) We should have a discussion about this before going forward with CNAME handling in the authoritative server.

Briefly: An authoritative server can believe itself to be authoritative for zones X and Y when actually it's only authoritative for X. If we allow CNAME chains between two zones, we may be giving false information and claiming it's authoritative. For example, a query for www.foo.com/A comes in could get back www.foo.com/CNAME and www.bar.com/A, which would be wrong if this server wasn't actually authoritative for bar.com.

In my opinion we should not chain outside of a zone, including not below zone cuts. In the linked discussion, Paul Vixie argued that we should not chain at all, even within the zone.

Jinmei told me via jabber that the in-memory data source doesn't chase CNAME targets yet; in light of this I'd like to recommend that it not be taught to do so until we've had time to discuss the issue further. (Note that the existing query logic for the SQL data source does return CNAME chains; this should probably be smartened up later.)

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

Replying to vorner:

Replying to jinmei:

Replying to vorner:

  • Is the MemoryZone? the right place to check for singleton RRs? If we really want to check that, then I guess RRset itself should throw in that case, this is not specific to memory zones.

One difficulty is that we may not always be able to detect violation of singleton in the form of a single RRset. In general, a zone can be given two RRs of the same name and of a single type from different RRsets, so we need to do something within zones anyway.

Well, at some point they would be put together and at that point they would throw an exception.

I'm not so sure about that. We may not maintain the RRsets in the form of
dns::RRsets object internally. For example, we may want to more
space-efficient representation like #404 and merge RRsets using the
special representation. (Of course we could still rely on the RRset class by
converting internal data to an RRset object and then performing merge on it.
That would be a design question about the tradeoff between speed and
logic consolidation).

Whether or not we should do this check at the level of RRset::addRdata() is a different question, and it's probably a good idea. I think it's a matter of separate ticket/task, though.

Did you create the ticket or should I?

I've created it. #525.

Thanks. It seems OK now, so merge, please.

Okay, thanks for the review. Branch merged. For task management purposes
I'll do:

  • close this ticket
  • reduce the "estimation" from 5 to 3 due to the subtasking (3 is my personal opinion, still not really sure about how we should do in such a case)
  • open a new one for the other half of this task, and give it an estimation of 3 (again, it's my personal opinion and not sure if this is a valid way)

comment:14 Changed 8 years ago by jinmei

  • Estimated Difficulty changed from 5.0 to 3.0
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.