Opened 3 years ago

Closed 3 years ago

#5097 closed enhancement (complete)

DhcpX: Migrate Pools configuration

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea 1.2 - Mozilla Milestone 1
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5019, #5037
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0.5
Total Hours: 4.5 Internal?: no

Description


Subtickets

Change History (15)

comment:1 Changed 3 years ago by tomek

  • Parent Tickets set to 5019, 5037

comment:2 Changed 3 years ago by fdupont

  • Owner set to fdupont
  • Status changed from new to accepted

comment:3 Changed 3 years ago by fdupont

  • Owner fdupont deleted
  • Status changed from accepted to assigned

Giving up for the moment: it seems it should be done by #5019 and co with other global parameters...

comment:4 Changed 3 years ago by fdupont

  • Summary changed from DhcpX: Migrate DHCPv4-over-DHCPv6 configuration to DhcpX: Migrate Pools configuration

Changed Dhpcv4-over-DHCPv6 into Pools.

comment:5 Changed 3 years ago by fdupont

  • Owner set to fdupont
  • Status changed from assigned to accepted

comment:6 Changed 3 years ago by fdupont

  • Owner changed from fdupont to UnAssigned
  • Status changed from accepted to reviewing

Done. Added missing user-context. Ready for review.

comment:7 Changed 3 years ago by fdupont

Added some checks in the code with corresponding unit tests. Now #3956 should be covered.
Still under review (last commit is 6871fc5b14320ff1e514037a0cc63678b47de9e9).

comment:8 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:9 follow-up: Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 4
  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 4

Thanks for implementing this ticket. In general, you did a very good
job, but I have a couple of comments:

dhcp_parsers.h
I don't like the fact PoolParser has PoolStoragePtr passed to
its constructor and address family passed to parse() method.
I think it should be the other way around. Right now when
you call parse(), you don't know where the parsed object will
be stored. You need to take a look at the parser implementation.

If you have some v4/v6 transition technology in mind that is
expected to encounter both v4 and v6 pools, then perhaps
passing both storate pointer and address family to parse() would
be the way to go?

dhcp_parser.cc
The code added in parse() casts to int first and then checks
if the range in in 0..255 range. There are couple problems with that.
First, the comment above says that the range will be checked
by Pool constructor. If you want to add an explicit check, please
remove the comment. Second, if you want to keep this check, please
update it to allow 0..128 range. Third, if it's out of range,
the exception should contain an explanation, not an empty string.

src/bin/dhcp6/json_config_parser.cc
You added PdPoolParser::getUint8. This method may be useful for
other parsers. Can you move it to SimpleParser class?

I don't think the require_ method is necessary at all. You can use
SimpleParser::getString(). It will throw if the parameter is not
there. The BOOST_FOREACH at the beginning of the PdPoolParser::parse
can be replaced with code similar to this:

addr_str = getString(pd_pool, "prefix");
delegated_len = getInteger(pd_pool, "delegated-len");
...

get{String,Integer,Boolean} methods will throw if requested parameter
is missing. This way you don't need require_ checks anymore.

One minor complication here is that there are several parameters that
are optional: excluded-prefix-len, option-data, user-context. Maybe
we need couple methods for getting optional parameters in the
SimpleParser class?

PdPoolListParser should not have a constructor at all. The pools
storage pointer should be passed to the parse() method as parameter.
That way you can probably move the PdPoolParser out of the foreach
loop, so it won't be instantiated with every loop iteration.

Also, the parameter of the parse must not end with underscore. That's
reserved for member fields.


Can you add an example of user-context parameter to doc/examples/
kea6/hooks.json? ParserTest.file in src/bin/dhcp6/tests/parser_unittest.cc
will try to load it.

I don't see option-data in PD pool in any examples. Can you add it?
The best place to add such a thing would be doc/examples/kea6/multiple-
options.json

One of the reasons for this migration is to go over all parameters we
support and make sure we have examples for them. It's also a good
unit-test case as examples are loaded in unit-tests.

comment:10 in reply to: ↑ 9 Changed 3 years ago by fdupont

Replying to tomek:

Thanks for implementing this ticket. In general, you did a very good
job, but I have a couple of comments:

dhcp_parsers.h
I don't like the fact PoolParser has PoolStoragePtr passed to
its constructor and address family passed to parse() method.
I think it should be the other way around. Right now when
you call parse(), you don't know where the parsed object will
be stored. You need to take a look at the parser implementation.

If you have some v4/v6 transition technology in mind that is
expected to encounter both v4 and v6 pools, then perhaps
passing both storate pointer and address family to parse() would
be the way to go?

=> the way simpleparsers are used allows to pass context parameters
as the storage and/or the family in the constructor or the parse
method so I can change it too what you'd like.

dhcp_parser.cc
The code added in parse() casts to int first and then checks
if the range in in 0..255 range. There are couple problems with that.
First, the comment above says that the range will be checked
by Pool constructor.

=> there are two ranges: the representation range given by
the integer type, and the range which the parameter can take.
The first must be checked before the value is cast in particular
for unsigned integer types. The second is the responsibility
of the Pool code.

If you want to add an explicit check, please
remove the comment. Second, if you want to keep this check, please
update it to allow 0..128 range.

=> I disagree but I believe it is more a problem of
terminology.

Third, if it's out of range,
the exception should contain an explanation, not an empty string.

=> it is useless to add an explanation as the the exception is handled
in the catch a few lines later.

src/bin/dhcp6/json_config_parser.cc
You added PdPoolParser::getUint8. This method may be useful for
other parsers. Can you move it to SimpleParser class?

=> with pleasure but in this case I have a template which
is more generic (and will avoid the 0..128 vs 0..255 issue).

I don't think the require_ method is necessary at all. You can use
SimpleParser::getString(). It will throw if the parameter is not
there.

=> getString does not throw a syntax error. BTW if you like
I can also add the require_ method to the SimpleParser class
(it just needs a better name).

The BOOST_FOREACH at the beginning of the PdPoolParser::parse
can be replaced with code similar to this:

addr_str = getString(pd_pool, "prefix");
delegated_len = getInteger(pd_pool, "delegated-len");
...

=> no, this does not do the right thing!

One minor complication here is that there are several parameters that
are optional: excluded-prefix-len, option-data, user-context. Maybe
we need couple methods for getting optional parameters in the
SimpleParser class?

=> to be clear: getXXX in the SimpleParser are broken so
I am afraid I'll have a concern to add more brokenness.

PdPoolListParser should not have a constructor at all. The pools
storage pointer should be passed to the parse() method as parameter.
That way you can probably move the PdPoolParser out of the foreach
loop, so it won't be instantiated with every loop iteration.

=> same than for PoolParser?

Also, the parameter of the parse must not end with underscore. That's
reserved for member fields.

=> I'll fix it


=> the other comments are about new unit tests (for sure it is a good idea
to add more in particular with concrete examples of what to add).

comment:11 Changed 3 years ago by fdupont

I addressed all the comments at the exception of the underscore (pools_ is a member of the subnet parser, I agree the name is not good in particular when the other argument is pools but this can be fixed only when migrating the subnet parser) and new unit tests.
I copied the template from master to the simple parser with BTW the obvious reason why the getXXX are broken: they can't raise the right exception because it is not yet defined (the template does not have this problem as it is enough to pass it).
Commit is c5d913c6384461ae413040f3f1cbd201b4674042

I keep the ticket for adding new unit tests.

comment:12 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

Added the tests. Ready for another round...

comment:13 Changed 3 years ago by fdupont

I added another template du SimpleParser. My idea is to move the getXXX tools for D2ClientConfigParser to members.
It should be cleaner but I don't know when to do it, e.g. during merging or as part as a final cleanup or in a dedicated (and to be reviewed) branch?

comment:14 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 4 to 0.5
  • Owner changed from tomek to fdupont
  • Total Hours changed from 4 to 4.5

I have reviewed your latest changes. They look fine. Thanks for doing them. The code is ready to go.

The code builds ok and unit-tests pass on Ubuntu 16.04.1 x64.

comment:15 Changed 3 years ago by fdupont

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

Merged (with a pass to use SimpleParser templates). Closing.

Note: See TracTickets for help on using tickets.