Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#3362 closed defect (fixed)

d2::NameRemoveTransaction should check for NXRRSET not NXDOMAIN

Reported by: tmark Owned by: marcin
Priority: high Milestone: Kea0.9
Component: ~dhcp-ddns(obsolete) Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 4 Add Hours to Ticket: .5
Total Hours: 3 Internal?: no

Description

When checking the DNS RCODE after attempting to remove either forward or reverse DNS entries, NameRemoveTransaction? should check
for the value NXRRSET, not NXDOMAIN.

The pre-requisite tests are "RRSet exists" checks not "Name exists"
checks, and hence are using the wrong RCODE value. This causes
deletes for entries that are not there (i.e. RCODE = NXRRSET) as
failures when they should be treated as success.

Subtickets

Change History (7)

comment:1 Changed 6 years ago by tmark

  • Estimated Difficulty changed from 0 to 4

comment:2 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 0 to 2
  • Owner set to UnAssigned
  • Status changed from new to reviewing

This change was straight forward. The NameRemoveTransaction code was already
structured to treat RCODEs indicating the DNS data to be removed doesn't exist
as success. It was merely checking for the wrong return value. The change
simply replaces checks for NXDOMAIN with checks for NXRRSET in the appropriate
places and tests.

This has been tested successfully against a BIND9 server.

7xx.    [bug]      tmark
    b10-dhcp-ddns now treats a DNS server response code of NXRRSET as a
    successful outcome when processing a request to remove DNS data.  This
    corrects a defect in which the server would incorrectly fail a request
    to remove DNS data when the DNS server's response was NXRRSET.
    (Trac #3362, git TBD)

comment:3 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:4 follow-up: Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 2 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 0 to 1

Reviewed commit 72d508acc50c642e4958937532b3728bdccf5a75

It may be useful to add some reference to the RFC2136 (Section 3.2.3?) in the code to explain why NXRRSET is a success. I know that there is the comment saying that this signal means ''there are no RRs for the FQDN'' but there is no reference to appropriate text.

Regarding the ChangeLog. The word ''server'' is a little confusing:

This corrects a defect in which the server would incorrectly fail a...

You mean: DHCP server, not a DNS server? DNS server doesn't make sense. Is b10-dhcp-ddns a server? Perhaps use ''b10-dhcp-ddns'' instead of ''server''? Or maybe ''DDNS module''?

It would be interesting to see if other DNS servers would return the same code. Did we consider testing DDNS against any other DNS server, e.g. dnsmasq? It is not the task for this ticket obviously, just an idea for testing.

comment:5 in reply to: ↑ 4 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 1 to .5
  • Owner changed from tmark to marcin
  • Total Hours changed from 1 to 2

Replying to marcin:

Reviewed commit 72d508acc50c642e4958937532b3728bdccf5a75

It may be useful to add some reference to the RFC2136 (Section 3.2.3?) in the code to explain why NXRRSET is a success. I know that there is the comment saying that this signal means ''there are no RRs for the FQDN'' but there is no reference to appropriate text.

I added commentary citing RFCs and such.

Regarding the ChangeLog. The word ''server'' is a little confusing:

This corrects a defect in which the server would incorrectly fail a...

You mean: DHCP server, not a DNS server? DNS server doesn't make sense. Is b10-dhcp-ddns a server? Perhaps use ''b10-dhcp-ddns'' instead of ''server''? Or maybe ''DDNS module''?

Yes, it should use "b10-dhcp-ddns" instead.

7xx.    [bug]      tmark
    b10-dhcp-ddns now treats a DNS server response code of NXRRSET as a
    successful outcome when processing a request to remove DNS data.  This
    corrects a defect in which b10-dhcp-ddns would incorrectly fail a request
    to remove DNS data when the DNS server's response was NXRRSET.
    (Trac #3362, git TBD)

It would be interesting to see if other DNS servers would return the same code. Did we consider testing DDNS against any other DNS server, e.g. dnsmasq? It is not the task for this ticket obviously, just an idea for testing.

I have thought about this, and on a broader scale we should test D2 with other DNSs, but not
under the effort of this ticket.

comment:6 Changed 6 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 2 to 3

Changes merged with commit # d3c971ea33bbcb43f2c66207a04eab43c91399e6
Change Log entry 768 created.

Last edited 6 years ago by tmark (previous) (diff)

comment:7 Changed 5 years ago by hschempf

  • Milestone changed from Kea1.0 to Kea0.9
  • Version set to git
Note: See TracTickets for help on using tickets.