BIND 10 #3232: Kea6: Develop REBIND support
BIND 10 Development
do-not-reply at isc.org
Mon Mar 10 15:31:16 UTC 2014
#3232: Kea6: Develop REBIND support
-------------------------------------+-------------------------------------
Reporter: tomek | Owner:
Type: enhancement | marcin
Priority: medium | Status:
Component: Unclassified | reviewing
Keywords: | Milestone: DHCP-
Sensitive: 0 | Kea0.9-alpha
Sub-Project: DHCP | Resolution:
Estimated Difficulty: 0 | CVSS Scoring:
Total Hours: 44 | Defect Severity: N/A
| Feature Depending on Ticket:
| Add Hours to Ticket: 5
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by tomek):
* hours: 4 => 5
* owner: tomek => marcin
* totalhours: 39 => 44
Comment:
'''src/bin/dhcp6/tests/dhcp6_client.cc'''
I like the concept of that class a lot. If we extend it in the right
direction, it will be a huge added benefit for our unit-tests.
It is not clear from the documentation if the functions doSARR(),
doSolicit(), doRebind() do any validation and how they behave if some
error conditions are detected (throw exceptions? use gtest macros?
silently ignore?).
Constructor: I'd like the NakedDhcpv6Srv being passed as an (optional)
parameter. If its value is NULL, then it is initialized in the
constructor. It is reasonable to assume that the tests would follow
the natural logic:
- create the server
- set server configuration
- "run" the client
- do whatever exchanges the test is checking
Also the constructor (or perhaps a comment for the whole class) should
mention what sort of configuration is being used. Alternatively, it
should point out that the actual values are set in the ctor.
applyConfiguration() - Somehow that name does not sound right. How
about processReceivedConfiguration() or processConfiguration()? If you
want to keep the name, I won't insist, but please at least update the
doxygen description.
useRelay() - it could possibly take a parameter that specifies the
relay link-addr.
getLease(), getStatusCode() should do offset check. (or use at()
method that will throw if trying to access beyond buffer)
sendMsg() - the comment for that function should clearly state that
is also triggers the server processing of that sent message.
I like that the code does on-wire packing. We used to not bother to do
that and it bitten us in the least convenient moment...
'''src/bin/dhcp6/tests/dhcp6_client.cc'''
Can you add comments to the last 3 closing brackets?
{{{
} // end of namespace isc::dhcp::test
} // end of namespace isc::dhcp
} // end of namespace isc
}}}
Dhcp6Client::applyConfiguration() the condition in line 75
{{{
prefix_len == 128 ? Lease::TYPE_NA : Lease::TYPE_PD)
}}}
is fishy. What if we want to check if prefixes with length 128 can be
delegated? There's much better way to detect if this is address or
prefix. Use IA type (is it IA_NA||IA_TA or IA_PD) or the option code
within
it (IAADDR or IAPREFIX). That code checking looks a bit odd. If I were
coding it, it would look like this:
{{{
for (all options in IA) {
switch (option code) {
case IAPREFIX:
if (ia->getType() != IA_PD) {
isc_throw("Received prefix in option other than IA_PD");
}
prefix = dynamic_pointer_cast<Option6IAPrefix>(option);
do something;
break;
case IAADDR:
addr = dynamic_pointer_cast<Option6IAAddr>(option);
/// @todo: extend this into IA_TA once we start supporting
/// temp. addrs
if (ia->getType() != IA_NA) {
isc_throw("Received address in option other than IA_NA");
}
do something;
break;
case STATUSCODE:
do something with status code;
break;
default:
isc_throw("Received unknown option in IA.");
}
}
}}}
When creating the lease later in the same function, please add a brief
comment that the lease is assumed to belong to a subnet with subnet-id
0. That's a reasonable assumption for now, but it may fail once we
start testing fancier scenarios with that class.
applyLease() - that function is not going to work properly with
NA+PD. IAID for IA_NA and for IA_PD are separate spaces, i.e. IA_NA
with iaid=1 is not the same as IA_PD with iaid=1.
The code should eventually have a clause that removes leases if
received with lifetimes set to 0. It's probably not needed for this
ticket, so a simple @todo will be sufficient for now.
copyIAs(), copyIAsFromLeases() - please add @todo for IA_TA.
generateDUID() - you use DUID-LLT, but the time is actually all
zeros. Perhaps it would be better to just randomize it? Or, if you
want to maintain the same value for 100% test reproductability, just
randomize the value once and store it in the test? I don't have any
strong opinions here. Using the same DUID in every test may have some
unintended cross-test implications. Catching those are both good and
bad (good because they would show flaws in our test environment and
bad, because spending more and more time on unit-tests slows down
the development).
'''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
testUnicast: It's a good test, but it has a flaw. The unicast is
allowed only if the server is configured to send server-unicast
option. If it's not configured, it should be reject/status-code with
UseMulticast for every client originated packet.
It is ok to accept RELAY-FORW even if there is no server-unicast
option configured.
There should be also a test that checks that the server does not drop
the packet, but sends back an ADVERTISE/REPLY with status
code=UseMulticast. As this is not the objective of this ticket, a
@todo is the way to go for now. As a side comment, it would be interesting
to observe how that would affect our Forge pass-rate as there are
quite a few tests that fail due to that.
'''src/bin/dhcp6/tests/rebind_unittest.cc'''
It's a good idea to split those tests to a separate file.
REBIND_CONFIGS structure: Can you add a one-line comment for each
configuration? It's a bit difficult to compare 2 configurations that
are 2 screens away and pick up the subtle differences. I was thinking
about something like
{{{
// Configuration 0: 2 subnets, address only (2001:db8:1/64,
2001:db8:2::/64)
// Configuration 1: same as 0, but different subnets ((2001:db8:4::/64,
2001:db8:4::/64)
// Configuration 2: ...
}}}
RebindTest::configure(): if you agree with my proposal for extending
client constructor to accept Dhcpv6Srv pointer, you could get rid of
this method and use Dhcpv6SrvTest::configure() method directly.
Which is not on this branch yet, because it was branched before
configure() was merged to master. So it makes my comment somewhat
useless, unless you want to do master->trac3232 merge, then update
then trac3232->master merge.
RebindTest::requestLease() - please add sanity checks for
config_index.
General comment for all tests in rebind_unittest.cc:
Please add one line explanation for each test purpose.
relayedClientChangingAddress test should have an assert that checks
that the new address 3000::100 is really different than what was
received.
Since the test coverage is already extensive, it is ok to skip the
following for now (and do @todo instead):
There is no test for direct client changing address.
directClientPDChangingPrefix is nice, but it's only half of the
story. Client could also attempt to rebind with the same prefix, but
different length.
There is no test for relayed clients with PD.
----------
I think section 18.6 (Support DHCPv6 standards) could mention the work you
did here.
I believe that some of the changes you did here are described in stateful-
issues draft, right?
And even if they aren't they will be in upcoming -06 of stateful-issues.
So it is
fair to mention that Kea supports it (at least the part about REBIND).
--
Ticket URL: <http://bind10.isc.org/ticket/3232#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list