Opened 6 years ago

Closed 6 years ago

#3295 closed enhancement (complete)

Refactor DHCPv6 code which processes Client FDQN option.

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

Description

The DHCPv6 server code, which processes the Client FQDN option has to be refactored. Currently, the FQDN option is not appended to the server's response until the very end of message processing. At the end of message processing we check if option was requested or not. If it was, it is appended to the response.

We think it is better to append the option to the response as soon as the option is generated. This will eliminate the need to pass the option in a separate variable all the way through different functions. This will simplify the code.

Subtickets

Change History (13)

comment:1 Changed 6 years ago by marcin

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

comment:2 Changed 6 years ago by marcin

  • Estimated Difficulty changed from 0 to 24

comment:3 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 0 to 6
  • Total Hours changed from 0 to 6

comment:4 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 6 to 8
  • Total Hours changed from 6 to 14

comment:5 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 8 to 6
  • Total Hours changed from 14 to 20

comment:6 Changed 6 years ago by marcin

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

Please note, that the code to be reviewed is on trac3295_2, NOT on trac3295!

Here is a summary of changes to the dhcp6_srv code:

  • The function processing client FQDN doesn't return client FQDN option but it appends it to the message being sent back to a client. Other functions use the object representing server's message to retrieve the fqdn data.
  • Allocation Engine returns the old lease (when new lease is acquired or a lease is renewed). The old lease is then compared with the new lease to make DDNS related decisions.
  • Allocation Engine doesn't update the lease for Solicit.
  • Added unit test which checks that appropriate NCRs are removed when reusing an old lease.
  • Among many IA_NAs and IAAddrs, only the first one is picked to create NCR.
  • Generation of an FQDN from leased IPv6 address when empty FQDN is sent by a client or server is configured to override client's notion of FQDN.
  • Created a convenience function which checks FQDN data between leases for equality.

comment:7 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 6 to 10
  • Total Hours changed from 20 to 30

comment:8 Changed 6 years ago by marcin

Forgot to add ChangeLog entry:

XXX.	[func]		marcin
	b10-dhcp6: Refactored the code which is processing Client FQDN option.
	The major user-visible change is that server generates DDNS
	NameChangeRequest for the first IPv6 address (instead of all)
	acquired by a client. Also, the server generates fully qualified domain
	name from acquired IPv6 address, if the client sends an empty name in
	Client FQDN option.
	(Trac# 3295, git abc)

comment:9 follow-up: Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 10 to 5
  • Owner changed from UnAssigned to marcin
  • Total Hours changed from 30 to 35

This was some complicated work, nice job.

cppcheck.sh was clean, I did NOT run the unittests through valgrind.


bin/dhcp6/dhcp6_src.cc

Dhcp6Srv::createNameChangeRequests()

At line 1056:

    // It is likely that client haven't included the FQDN option. In such case,
    // FQDN option will be NULL. This is valid state, so we simply return.

If DDNS is enabled we are always going to include the FQDN option in the
answer so it should be there. Suppose DDNS is enabled but a programming error
results in there being no FQDN option? This test would mask such a condition.

I would suggest only calling this method if DDNS is enabled and inside the method
throw an exception if there is no FQDN option.


bin/dhcp6/dhcp6_src.cc

Dhcp6Srv::createRemovalNameChangeRequests()

Rather than do an explicit check and log message for an empty hostname at
line 1138, couldn't you just let it process the conversion attempt at 1153?
writeFqdn should throw if passed an empty string and both tests guard against
a host name corrupted by outside intervention in the db.

Also, the log message at 1155 should probably include the lease address.


bin/dhcp6/dhcp6_srv.cc

Dhcp6Srv::generateFqdn()

At line 2449 you call updateLease6 even if the lease is not found. In other
words, it is not protected by the lease not null test at 2446. Really, if
the lease isn't found shouldn't you throw an exception? It means something
is rather wrong.


lib/dhcpsrv/alloc_engine.h

There is no commentary for Lease6Collection::updateFqdnData.


lib/dhcpsrv/tests/lease_unittest.c

TEST(Lease6, hasIdenticalFqdn)

Curiously this test uses Lease4 instances to compare against. Not quite
what you had in mind is it?


bin/dhcp6/tests/fqdn_unittest.cc

Cosmetic but...at line 305 you use the conditional (ternary) operator which is great, I love them for this type of thing, but I prefer to enclose them in parens like so:

        Option6ClientFqdn::DomainNameType fqdn_type = (hostname.empty() ?
            Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL);

This way the reader's mind knows the start an expression. It's up to you.


comment:10 in reply to: ↑ 9 ; follow-up: Changed 6 years ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

This was some complicated work, nice job.

cppcheck.sh was clean, I did NOT run the unittests through valgrind.

Thank you QA team. Isn't it your responsibility to run valgrind as well? ;) Just kidding.


bin/dhcp6/dhcp6_src.cc

Dhcp6Srv::createNameChangeRequests()

At line 1056:

    // It is likely that client haven't included the FQDN option. In such case,
    // FQDN option will be NULL. This is valid state, so we simply return.

If DDNS is enabled we are always going to include the FQDN option in the
answer so it should be there. Suppose DDNS is enabled but a programming error
results in there being no FQDN option? This test would mask such a condition.

I would suggest only calling this method if DDNS is enabled and inside the method
throw an exception if there is no FQDN option.

It is not how the code is working now. The processClientFqdn will not append Client FQDN option if there is no such an option in client's message. Client informs of its capability to process this option by sending it to the server (and adding its code in ORO). So, you can't expect that the FQDN option is always there.


bin/dhcp6/dhcp6_src.cc

Dhcp6Srv::createRemovalNameChangeRequests()

Rather than do an explicit check and log message for an empty hostname at
line 1138, couldn't you just let it process the conversion attempt at 1153?
writeFqdn should throw if passed an empty string and both tests guard against
a host name corrupted by outside intervention in the db.

I initially thought that checking for empty hostname is faster than forcing writeFqdn to thrown an exception. However, since this is a prgrammatic error it should be rare and thus it doesn't really matter.

Also, the log message at 1155 should probably include the lease address.

Added.


bin/dhcp6/dhcp6_srv.cc

Dhcp6Srv::generateFqdn()

At line 2449 you call updateLease6 even if the lease is not found. In other
words, it is not protected by the lease not null test at 2446. Really, if
the lease isn't found shouldn't you throw an exception? It means something
is rather wrong.

What was I thinking?! Fixed.


lib/dhcpsrv/alloc_engine.h

There is no commentary for Lease6Collection::updateFqdnData.

Yep. I am pretty sure I added this comment and I have no idea what happened to it. Added again.


lib/dhcpsrv/tests/lease_unittest.c

TEST(Lease6, hasIdenticalFqdn)

Curiously this test uses Lease4 instances to compare against. Not quite
what you had in mind is it?

Yes, this test was copied from the V4 test. Fixed.


bin/dhcp6/tests/fqdn_unittest.cc

Cosmetic but...at line 305 you use the conditional (ternary) operator which is great, I love them for this type of thing, but I prefer to enclose them in parens like so:

        Option6ClientFqdn::DomainNameType fqdn_type = (hostname.empty() ?
            Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL);

This way the reader's mind knows the start an expression. It's up to you.

My editor (everybody knows what it is), does odd things in terms of aligning parameters in multiple lines, when doing this change. But I forced it to accept it. :)


comment:11 Changed 6 years ago by marcin

  • Add Hours to Ticket changed from 5 to 3
  • Total Hours changed from 35 to 38

comment:12 in reply to: ↑ 10 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 3 to 2
  • Owner changed from tmark to marcin
  • Total Hours changed from 38 to 40

Replying to marcin:

Replying to tmark:

This was some complicated work, nice job.

cppcheck.sh was clean, I did NOT run the unittests through valgrind.

Thank you QA team. Isn't it your responsibility to run valgrind as well? ;) Just kidding.


bin/dhcp6/dhcp6_src.cc

Dhcp6Srv::createNameChangeRequests()

At line 1056:

    // It is likely that client haven't included the FQDN option. In such case,
    // FQDN option will be NULL. This is valid state, so we simply return.

If DDNS is enabled we are always going to include the FQDN option in the
answer so it should be there. Suppose DDNS is enabled but a programming error
results in there being no FQDN option? This test would mask such a condition.

I would suggest only calling this method if DDNS is enabled and inside the method
throw an exception if there is no FQDN option.

It is not how the code is working now. The processClientFqdn will not append Client FQDN option if there is no such an option in client's message. Client informs of its capability to process this option by sending it to the server (and adding its code in ORO). So, you can't expect that the FQDN option is always there.

As we discussed there I see now that there is a legitmate case where DDNS is
enabled but the client's request did NOT include the FQDN. This implies that
the client has no intererst if DDNS. In such a case it would be valid for
there to be no FQDN in the response.

Please add a test at the beginning of the method to reutrn if FQDN_ENABLE_UDPAT E is false or a todo stating a test should be added.


bin/dhcp6/dhcp6_src.cc

Dhcp6Srv::createRemovalNameChangeRequests()

Rather than do an explicit check and log message for an empty hostname at
line 1138, couldn't you just let it process the conversion attempt at 1153?
writeFqdn should throw if passed an empty string and both tests guard against
a host name corrupted by outside intervention in the db.

I initially thought that checking for empty hostname is faster than forcing writeFqdn to thrown an exception. However, since this is a prgrammatic error it should be rare and thus it doesn't really matter.

Also, the log message at 1155 should probably include the lease address.

Added.


bin/dhcp6/dhcp6_srv.cc

Dhcp6Srv::generateFqdn()

At line 2449 you call updateLease6 even if the lease is not found. In other
words, it is not protected by the lease not null test at 2446. Really, if
the lease isn't found shouldn't you throw an exception? It means something
is rather wrong.

What was I thinking?! Fixed.


lib/dhcpsrv/alloc_engine.h

There is no commentary for Lease6Collection::updateFqdnData.

Yep. I am pretty sure I added this comment and I have no idea what happened to it. Added again.

You must have used a "disappearing font". I hate it when that happens.


lib/dhcpsrv/tests/lease_unittest.c

TEST(Lease6, hasIdenticalFqdn)

Curiously this test uses Lease4 instances to compare against. Not quite
what you had in mind is it?

Yes, this test was copied from the V4 test. Fixed.


bin/dhcp6/tests/fqdn_unittest.cc

Cosmetic but...at line 305 you use the conditional (ternary) operator which is great, I love them for this type of thing, but I prefer to enclose them in parens like so:

        Option6ClientFqdn::DomainNameType fqdn_type = (hostname.empty() ?
            Option6ClientFqdn::PARTIAL : Option6ClientFqdn::FULL);

This way the reader's mind knows the start an expression. It's up to you.

My editor (everybody knows what it is), does odd things in terms of aligning parameters in multiple lines, when doing this change. But I forced it to accept it. :)


If you agree with my comments, I do not need to see this ticket again. Please proceed.

comment:13 Changed 6 years ago by marcin

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

Merged with commit aa1c94a54114e848c64771fde308fc9ac0c00fd0

Note: See TracTickets for help on using tickets.