Opened 6 years ago

Closed 6 years ago

#3191 closed defect (complete)

Kea4: siaddr field must be configurable

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20131016
Component: dhcp4 Version:
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: 0
Total Hours: 0 Internal?: no

Description

Yet another thing we discovered that is needed for the demo.

Subtickets

Change History (9)

comment:1 Changed 6 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

Ready for review. This code was written in a crazy pace. Expect the changes to not make much sense.

comment:2 Changed 6 years ago by tomek

  • Priority changed from very high to medium

comment:3 Changed 6 years ago by marcin

  • Owner changed from UnAssigned to marcin

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

  • Owner changed from marcin to tomek

Reviewed commit: e98ccfe63db14be0d9a027797ffd75885b296969

doc/guide/bind10-guide.xml
Typo: ''form the TFTP server''. It should be ''from the TFTP server''.

src/bin/dhcp4/config_parser.cc
Why do you use a raw pointer (subnet4) instead shared_ptr (subnet_) in invocations to setSiaddr?

        Subnet4* subnet4 = new Subnet4(addr, len, t1, t2, valid);
        subnet_.reset(subnet4);

        // Try global value first
        try {
            string next_server = globalContext()->string_values_->getParam("next-server");
            subnet4->setSiaddr(IOAddress(next_server));
        } catch (const DhcpConfigError&) {
            // don't care. next_server is optional. We can live without it
        }

        // Try subnet specific value if it's available
        try {
            string next_server = string_values_->getParam("next-server");
            subnet4->setSiaddr(IOAddress(next_server));
        } catch (const DhcpConfigError&) {
            // don't care. next_server is optional. We can live without it
        }

src/bin/dhcp4/dhcp4.spec
Trivial: Line 229-231 - odd alignment of lines

src/bin/dhcp4/tests/config_parser_unittest.cc
There are no negative tests for the siaddr. If you think it is too much burden, please add a todo comment. In particular I am thinking that invalid next-server (say abc) should result in configuration failure.

src/lib/dhcpsrv/subnet.cc
setSiaddr: Can you replace ''addr'' with ''address'' in the exception string.
Spurious commentary in lines 163 and 164

src/lib/dhcpsrv/subnet.h
Capital letters are encouraged. :-)

src/lib/dhcpsrv/tests/subnet_unittest.cc
I think that the description of the siaddr test is misleading. We barely check that we can set and get the value of the siaddr. Also typo ''handle''. Should be ''handled''.

That change deserves a ChangeLog.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 6 years ago by tomek

  • Owner changed from tomek to marcin

Replying to marcin:

Reviewed commit: e98ccfe63db14be0d9a027797ffd75885b296969

doc/guide/bind10-guide.xml
Typo: ''form the TFTP server''. It should be ''from the TFTP server''.

Fixed.

src/bin/dhcp4/config_parser.cc
Why do you use a raw pointer (subnet4) instead shared_ptr (subnet_) in invocations to setSiaddr?

Updated to Subnet4Ptr. I can't use subnet_ directly, because it is SubnetPtr? object and setSiaddr() method is available in Subnet4 object only, not in Subnet.

src/bin/dhcp4/dhcp4.spec
Trivial: Line 229-231 - odd alignment of lines

Fixed.

src/bin/dhcp4/tests/config_parser_unittest.cc
There are no negative tests for the siaddr. If you think it is too much burden, please add a todo comment. In particular I am thinking that invalid next-server (say abc) should result in configuration failure.

Added test that checks 3 negatives: garbage (a.b.c.d), IPv6 address and empty string.

src/lib/dhcpsrv/subnet.cc
setSiaddr: Can you replace ''addr'' with ''address'' in the exception string.
Spurious commentary in lines 163 and 164.

Removed.

src/lib/dhcpsrv/subnet.h
Capital letters are encouraged. :-)

Capitalized a couple of them. Not sure if there are any left.


src/lib/dhcpsrv/tests/subnet_unittest.cc
I think that the description of the siaddr test is misleading. We barely check that we can set and get the value of the siaddr. Also typo ''handle''. Should be ''handled''.

Description updated.

That change deserves a ChangeLog.

ChangeLog? proposal added.

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

  • Owner changed from marcin to tomek

Replying to tomek:

Replying to marcin:

Reviewed commit: e98ccfe63db14be0d9a027797ffd75885b296969

That change deserves a ChangeLog.

ChangeLog? proposal added.

Looks good. Other changes look good also.

We have earlier noticed that the global values of siaddr were overridden by the subnet-specific value of 0.0.0.0. I am a bit surprised actually, because I thought that default values are not working properly in bind10 and even though you have a default, you have to specify non-optional parameters. Nevertheless, please double check if it works or not. Also, I think that the next-server should not have a default per-subnet value at all because if defaults get working, the subnet-specific value will always override the global.

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

  • Owner changed from tomek to marcin

Replying to marcin:

We have earlier noticed that the global values of siaddr were overridden by the subnet-specific value of 0.0.0.0. I am a bit surprised actually, because I thought that default values are not working properly in bind10 and even though you have a default, you have to specify non-optional parameters. Nevertheless, please double check if it works or not. Also, I think that the next-server should not have a default per-subnet value at all because if defaults get working, the subnet-specific value will always override the global.

Fixed that. But it is not really testable in unit-tests, at least not without major refactor. That is due to the way how we create config file:

    string config = "{ some JSON config here }";
    ElementPtr json = Element::fromJSON(config);

This does not take into account the .spec definitions. So eventually we'll need a function like fromJSONbutApplySpecFileDefaultsFirst(). Added ticket #3205 for this.

comment:8 Changed 6 years ago by marcin

  • Owner changed from marcin to tomek

Reviewed commit d8d20c9781e6a644c85339b966c5c6a31ad24a59

Changes look good. Please fix the typo in Dhcpv4SrvTest::nextServerOverride: ''messagey'' and merge it to master.

comment:9 Changed 6 years ago by tomek

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

Thanks for the review.

Fixed, merged, closed.

Note: See TracTickets for help on using tickets.