BIND 10 #3232: Kea6: Develop REBIND support
BIND 10 Development
do-not-reply at isc.org
Tue Mar 11 19:26:35 UTC 2014
#3232: Kea6: Develop REBIND support
-------------------------------------+-------------------------------------
Reporter: tomek | Owner: tomek
Type: enhancement | Status:
Priority: medium | reviewing
Component: Unclassified | Milestone: DHCP-
Keywords: | Kea0.9-alpha
Sensitive: 0 | Resolution:
Sub-Project: DHCP | CVSS Scoring:
Estimated Difficulty: 0 | Defect Severity: N/A
Total Hours: 49.5 | Feature Depending on Ticket:
| Add Hours to Ticket: 5.5
| Internal?: 0
-------------------------------------+-------------------------------------
Changes (by marcin):
* hours: 5 => 5.5
* owner: marcin => tomek
* totalhours: 44 => 49.5
Comment:
Replying to [comment:7 tomek]:
> '''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.
Great! I think it should ultimately be easier to extend test coverage and
create 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?).
Added comments.
>
> 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
I added additional constructor that accepts the !NakedDhcpv6Srv
>
> 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.
>
I added a list of parameters being initialized by the constructors.
> 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.
I renamed it to applyRcvdConfiguration. The DHCP is about supplying
configuration, after all. !''Rcvd!'' indicates that client has got it from
external source.
>
> useRelay() - it could possibly take a parameter that specifies the
> relay link-addr.
Added optional parameter.
>
> getLease(), getStatusCode() should do offset check. (or use at()
> method that will throw if trying to access beyond buffer)
Note that this is a test class, not a part of the released product. I
don't think there is a need to sanity check everything. I added a warning
message. Note, that there is another function which returns the number of
elements so the caller should be aware what is the maximal index it can
use.
>
> sendMsg() - the comment for that function should clearly state that
> is also triggers the server processing of that sent message.
Added.
>
> 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...
Me too! :)
>
> '''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
> }}}
Added.
>
> 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.");
> }
> }
> }}}
Yes. This function became a bit ugly as a result of my attempt to not
copy-paste too much for each processed IA. I restructured this function a
bit and it now looks nicer.
>
> 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.
Sorry, I didn't get that point.
>
> 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.
>
Ok. I now distinguish the PDs and NAs.
> 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.
From the unit test standpoint it is good that leases with zero-lifetimes
are not immediately removed because the unit test want to check that
lifetimes are 0.
Perhaps there will need to be a public function that can be explicitly
called to remove leases of this kind. This class will need to be extended
anyway, so I am not sure we need todo for evernything?
>
> copyIAs(), copyIAsFromLeases() - please add @todo for IA_TA.
Added.
>
> 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).
I now randomize that value.
>
> '''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.
Well, when talking about the Rebind message it must always be discarded if
sent to a unicast and not relayed, I think:
{{{
A server MUST discard any Solicit, Confirm, Rebind or
Information-request messages it receives with a unicast destination
address.
}}}
This is from section 15 of RFC3315.
>
> It is ok to accept RELAY-FORW even if there is no server-unicast
> option configured.
I added a test for that.
>
> 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.
Advertise/Reply is out of scope here.
>
> '''src/bin/dhcp6/tests/rebind_unittest.cc'''
> It's a good idea to split those tests to a separate file.
Thanks. Especially that we will add more tests to it for stateful issues.
>
> 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: ...
> }}}
Added. Also removed Config4 which appeared to be unused.
>
> 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.
I don't want to merge that back and forth. I will remove it once I have a
chance in another ticket.
>
> RebindTest::requestLease() - please add sanity checks for
> config_index.
I added sanity checks. Note that it is just a test which has a precise
control over what is passed to which function so I consider this nice to
have change.
>
> General comment for all tests in rebind_unittest.cc:
> Please add one line explanation for each test purpose.
Ooops. Why did I forget about it? Added.
>
> relayedClientChangingAddress test should have an assert that checks
> that the new address 3000::100 is really different than what was
> received.
Added.
>
> Since the test coverage is already extensive, it is ok to skip the
> following for now (and do @todo instead):
Thank you! :)
>
> 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 added a couple of todos.
>
> ----------
> 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).
It is not supporting the stateful issues yet. The key change in stateful
issues is that server may allocate leases upon Renew or Rebind, which we
don't support here. I decided to write this up in stateful issues before
implementing it.
--
Ticket URL: <http://bind10.isc.org/ticket/3232#comment:8>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development
More information about the bind10-tickets
mailing list