Opened 2 years ago

Closed 2 years ago

#5369 closed defect (fixed)

DDNS entries are being removed erroneously

Reported by: marcosgildavid Owned by: tmark
Priority: medium Milestone: Kea1.3-final
Component: Unclassified Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 1
Total Hours: 7 Internal?: no

Description

Reference:
https://lists.isc.org/pipermail/kea-users/2017-September/001262.html

  1. If you use replace-client-name = "always":

Upon renewal, we are incorrectly deciding the FQDN has changed and is blank, and are
doing a remove only. It does not matter whether the client sends the
generated name in the renewals or not.

  1. If you use replace-client-name = "when-not-present":

We commit this same basic mistake if the client omits the hostname on
renewals. If, however, they send the generated hostname on renewals we
correctly assert they are equal and do not alter DNS.

Subtickets

Change History (8)

comment:1 Changed 2 years ago by tmark

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

comment:2 Changed 2 years ago by tmark

  • Milestone changed from Kea-proposed to Kea1.3-final

comment:3 Changed 2 years ago by tmark

  • Add Hours to Ticket changed from 0 to 6
  • Owner changed from tmark to Unassigned
  • Status changed from assigned to reviewing

Ticket is ready for review.

I moved the DNS remove logic from inside the allocation engine into the kea-dhcp4 server, so it can be done after we've determined what the FQDN should be for the new lease.

ChangeLog?:

13xx.   [bug]   tmark
    Corrected logic that was causing kea-dhcp4 to remove DNS entries
    when renewing leases with generated FQDN names.  Prior to this
    the server was incorrectly scheduling DNS removal when configured
    to generate the client's FQDN.
    (Trac #5369, git TBD)

comment:4 Changed 2 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:5 Changed 2 years ago by fdupont

  • Owner changed from fdupont to tmark

I fixed comments in new unit tests so please pull and review my changes.
Patch OK.

comment:6 follow-up: Changed 2 years ago by fdupont

BTW I tried the FQDN unit tests with the current code and only one test fails:

[ RUN      ] NameDhcpv4SrvTest.processRequestFqdnEmptyDomainName
fqdn_unittest.cc:913: Failure
      Expected: 0
To be equal to: d2_mgr_.getQueueSize()
      Which is: 1
[  FAILED  ] NameDhcpv4SrvTest.processRequestFqdnEmptyDomainName (7 ms)

And in particular processTwoRequestsHostname and processRequestRenewHostname pass
when I expected they would not...

comment:7 in reply to: ↑ 6 Changed 2 years ago by tmark

Replying to fdupont:

BTW I tried the FQDN unit tests with the current code and only one test fails:

[ RUN      ] NameDhcpv4SrvTest.processRequestFqdnEmptyDomainName
fqdn_unittest.cc:913: Failure
      Expected: 0
To be equal to: d2_mgr_.getQueueSize()
      Which is: 1
[  FAILED  ] NameDhcpv4SrvTest.processRequestFqdnEmptyDomainName (7 ms)

Yes, this one should've fail as I extended to do a renewal.

And in particular processTwoRequestsHostname and processRequestRenewHostname pass
when I expected they would not...

I added those two tests to increase scenario coverage. I knew the current code handles these scenarios correctly but we weren't explicitly testing them. I wanted to ensure that the new code didn't break these cases.

Changes merged with git 18f57f502f1b9fb5bf7ef5ab995ddda60006fd39
Added ChangeLog? entry 1305.

Ticket is complete.

comment:8 Changed 2 years ago by tmark

  • Add Hours to Ticket changed from 6 to 1
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 0 to 7
Note: See TracTickets for help on using tickets.