Opened 5 years ago

Last modified 4 years ago

#3491 reviewing enhancement

Context-based DHCP service (contributed patch)

Reported by: tomek Owned by: contributor
Priority: medium Milestone: Outstanding Tasks
Component: dhcp4 Version: git
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 (last modified by tomek)

This ticket will cover parts of the patch submitted by Shawn Lewis and David Gong that are related to context-based DHCP service.

Subtickets

Attachments (1)

benu.7.16.patch (116.4 KB) - added by tomek 5 years ago.
Patch as contributed (covers many tickets)

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0

As discussed on Kea meeting (2014-07-16) we not include this ticket in 0.9, due to lack of available processing time. Therefore moving to 1.0.

comment:2 Changed 5 years ago by tomek

  • Description modified (diff)
  • Summary changed from Context-based DHCP service (submitted patch) to Context-based DHCP service (contributed patch)

Comments regarding context-based DHCP server.

First of all, I consider contexts a very powerful concept. It can be useful in many use cases
that you mentioned. Another use cases that was not mentioned may be possible deployment
scenarios for draft-ietf-dhc-dynamic-shared-v4allocation-01, which assumes that one
IPv4 lease is not a whole IPv4 address, but rather a fraction of it (each client is allowed to use
only specified port-set or port range). It's not exactly the same as other context use cases,
but it is somewhat similar in the sense that there will be multiple leases for the same address,
so address uniqueness is no longer guaranteed.

Implementing contexts is a difficult task, so please don't be discouraged if this part of your
patch will take extra time and work is merged.

One fundamental thing that is currently not addressed is the ability to store more than one
lease with the same address. ( when each lease belongs to a different context). It will be
mandatory to develop such capability for existing databases (at least one of them at the beginning,
but ultimately we'll have to extend them all), so we could run tests and debug the context capability.

Before we go into the details of my review, I'd like to discuss the data model.
Let's assume that we have 3 contexts.

Context A with vlan=10, MAC unspecified and 192.168.1.0/24 subnet.
Context B with vlan=20, MAC 00:01:02:03:04:05 and 192.168.1.0/24 subnet.
Context C with vlan=30, MAC 00:aa:bb:cc:dd:ee and 192.168.1.0/24 subnet.

Normally, such a configuration would be meaningless, as the subnets are the same. In fact, we
have plans to implement checks that subnets do not overlap. This is a useful sanity check in
normal (context-less) environment, but would prevent contexts from operating.
This is easy to solve. We need a global configuration parameter that is disabled by default
and, when specified, enables context mode. In that mode some sanity checks could be disabled.

Now, this configuration will be translated to 3 Subnet4 objects, each with its own subnet_id
(1,2 and 3, respectively). It may be useful to read my comments for subnet.cc|h below, when
I explain in detail why Subnet4Context is not needed. Each Subnet4 structure will represent
a subnet, but also a context. If you have 10 contexts, you'll have 10 Subnet4 structures.
Each of those Subnet4 object may have the same subnet and/or pool.

The aforementioned layout can be also used to conveniently access vlan, MAC and any other parameter
that will appear in the future. To differentiate between contexts, you can use a single subnet_id
Instead of using (vlan, mac) tuples.

Let's talks about how would the leases be organized in contexts. When client connects to a
network that is covered in context A, he'll get a lease that looks like this:
Lease4(addr=192.168.1.5, subnet_id=1)

Client in context B will get lease that looks similar to this:
Lease4(addr=192.168.1.8, subnet_id=2)

And so on. It is even possible that the client in context C will get the same address as
in other contexts:
LEase4(addr=192.168.1.5, subnet_id=3)

You proposed to keep context parameters in each lease. In my opinion this is not the right way to go.
The context parameters are a property of the subnet, not the lease. Think about a case
where you have a million clients in one context. All of them will have the same vlan and
the same relay's MAC address. So it doesn't make sense to keep it million times. Also, this
data organization is self-consistent: whatever is set in Subnet4 applies to all leases within it.
For example, if you want to change vlan, you just do it once (in Subnet4 object) and it applies
automatically to all leases with this context.

If we were to go with the data model you proposed, the data could become inconsistent, e.g.
your Subnet4 would have vlan=123, but the lease within this context could have vlan=456.

We may need to implement an extra method, something like CfgMgr::getSubnet4(subnet_id), but
it is trivial to do. If you need to access vlan and relay MAC parameters, you can obtain them from
Subnet4 structure. Subnet4 objects are uniquely identified by the subnet_id, so if you ever need
to get parameters for any given lease, you can do so using
CfgMgr::getSubnet4(lease4->subnet_id_)->vlan.

With that layout, many of the things you need are greatly simplified. You can use
getLease4(hwaddr, subnet_id) to obtain both regular and context leases, so
getLease4Context() is not needed at all. The new equality operators, the new Lease4Context
class with all its methods, are no longer needed.

Having said that, there are couple things that you probably haven't encountered yet
(or perhaps you did, but haven't dealt with in your patch):

Issue 1: the call getLease4(addr) will no longer work if contexts are enabled, because the address used
to be unique, but it is not anymore. I think all the places that use that call
need to be modified:

OLD:

  lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr());

NEW:

if (!CfgMgr()::instance().contextMode()) {
   lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr());
} else {
   lease = LeaseMgrFactory::instance().getLease4(release->getCiaddr(),
                                                 subnet->subnet_id_);
}

It is important to keep the original call when context-mode is disabled, because in many
cases such a query is faster (database is indexed by addresses).

Issue 2: MySQL and Postgres databases assume that the address field is unique and they
use it as a primary key. In current configuration, they will not allow inserting leases with
duplicate addresses, even though they belong to separate subnets (contexts).

Now, on to the specific file modifications:

src/lib/dhcpsrv/subnet.cc|h
I think the concept of having a class derived from Subnet is ok, but there's a better way to
handle context subnets. What if you added setContextField(), getContextField() and
getNumberOfContextFields() in the base Subnet class? This way it would be automatically
usable in IPv4 and IPv6. We'd also have 2 less classes to maintain. With this approach,
Subnet4 structure would represent a subnet, but also a context. So if you have 10 contexts,
there would be 10 Subnet4 structures.

My second comment is about vlan and relay mac address. There's already a structure for
those parameters. Please move them to Subnet::RelayInfo. As was mentioned in the PDF
you provided, there may be additional parameters in the future (second vlan in Q-in-Q mode,
GRE, IP-IP, etc.), so this will not be just merely vlan+mac and will grow over time. Therefore
it's important to design the API in a way that wouldn't require extending the list of parameters
every time we add new parameter and just pass one structure. This point applies to other
classes, not just Subnet.

src/lib/asiolink/io_address.cc|h
IOAddress object is for IPv4 and IPv6 addresses. The HWAddr conversion does not belong here.
The convertHWAddrToString and onvertStringToHWAddr are not needed. They are already
implemented in HWAddr class as toText() and fromText(). If the code there doesn't do exactly
what you need, please tweak it rather than add new functions that do almost the same.

src/lib/dhcpsrv/alloc_engine.cc|h
Once you apply changes I proposed in subnet.cc|h, you'll need to tweak your alloc_engine
code as well. Fortunately, it will become simpler as well.

src/lib/dhcpsrv/lease_mgr.cc|h
If we follow the data model I described above, most (if not all) of your modifications are
not needed. That would make the code simpler and easier to maintain. Even if we decide,
that such calls are needed, they should be modified to not contain so many parameters.
This is not the problem of having too many parameters per se, but rather the problem lies in
the fact that the number of parameters will grow. Your code now supports vlan and relay MAC
address. But once you start supporting Q-in-Q, there may be 2 vlan IDs, there may be
IP address if you go with IP-IP tunnel, or some extra identifiers for GRE. It would be better
to pass one structure that describes all parameters. Subnet::RelayInfo? looks like a good
structure here. It needs to be extended with additional fields, but that's easy to do.

So instead of:

Lease4Ptr getLease4Context(const isc::dhcp::HWAddr& hwaddr, SubnetID subnet_id,
                           HWAddrPtr & relay_mac, uint16_t vlan);

It will looks more or less like:

Lease4Ptr getLease4Context(const isc::dhcp::HWAddr& hwaddr, SubnetID subnet_id, 
                           const RelayInfo& relay_details);

But I still think that if we implement the data model I proposed, the relay_details information
will not be needed at all, because the the subnet_id uniquely identifies the context/subnet,
so the relay_details are not needed at all. Hence you can use getLease4(hwaddr, subnet_id),
a call that is already there. Hence no new extra calls required.


There are several trailing whitespaces in the patch. Please try to avoid those. If you happen
to use emacs, there's covenient macro to remove them whitespace-cleanup. Other editors
provide similar capability.

Please try to limit lines width to no more than 100 characters. The coding guidelines
(see contributor's guide) say that it should be no more 80 characaters, but that's just an
old value that is no longer appropriate for C++ and its long names. We'll update the
coding guidelines in a near future.

Also, when generating a patch, make sure you specify the base revision it was based
upon. Right now the patch applies with only minor conflicts, but this may change in
the future.

This is somewhat related to context, so I thought I should mention it here:

In the comment for subnet4_preselect you mentioned that the selected subnet could potentially
be stored in some other place, not in CfgMgr. I have mixed feelings about this. But let's
assume for now that it will be possible. The only limitation is that subnet_id_ for all
subnets must be unique, regardless of their storage.

(The reason why I don't like the concept of having Subnets stored somewhere else than in
CfgMgr is that there are many, many use cases where the server engine selects a subnet.
For any given circumstances (the same relay, vlan, etc.), the code should always get the same
subnet.

comment:3 Changed 5 years ago by tomek

  • Status changed from new to reviewing

Changed 5 years ago by tomek

Patch as contributed (covers many tickets)

comment:4 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1
  • Version set to git

comment:5 Changed 5 years ago by tomek

  • Owner changed from UnAssigned to contributor

There are many things pointed out here, we're awaiting for an updated patch.

Reassigning to 'contributor' user to signify that fact.

comment:6 Changed 5 years ago by tomek

  • Milestone changed from Kea0.9.1 to Kea0.9.2

Moving to 0.9.2 as discussed on Kea weekly call today (2014-10-08).

If we hear back from the contributor and receive updated patch, we'll move it back to 0.9.1.

comment:7 Changed 5 years ago by hschempf

  • Milestone changed from Kea0.9.2 to DHCP Outstanding Tasks

comment:8 Changed 4 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to Outstanding Tasks

Milestone renamed

Note: See TracTickets for help on using tickets.