Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#5315 closed enhancement (complete)

subnet manipulation hook (subnetX-add, subnetX-del)

Reported by: tomek Owned by: tmark
Priority: medium Milestone: Kea1.3 beta
Component: management API Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Premium Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 4
Total Hours: 41 Internal?: no

Description

This is a continuation of #5314. Once the simple read-only operations are done, it's time to tackle the write operations: adding and removing subnets.

This ticket should cover the following:

  • subnet4-add, subnet6-add
  • subnet4-del, subnet6-del

Subtickets

Change History (10)

comment:1 Changed 2 years ago by marcin

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

comment:2 Changed 2 years ago by marcin

  • Add Hours to Ticket changed from 0 to 30
  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing
  • Total Hours changed from 0 to 30

There are lots of changes introduced by this ticket. The changes are both in Kea core and premium repository on their respective trac5315 branches. The reviewer trying to compile trac5315 in premium must have trac5315 checked out of the main Kea repo.

One of the major changes is that the CfgMgr's current configuration is now non-const so as it can be modified by the hooks library. But, this is in fact a very simple change and it is a start. The major issue with this ticket is that adding a new subnet requires that the hooks library has access to the subnet configuration parsers. So, the !Subnet4ConfigParser and !Subnet6ConfgParser had to be moved from the DHCPv4/v6 daemons to libdhcpsrv. That in turn required moving option parsers to the same place.

The next problem that had to be addressed was that the subnet configuration inherits some global configuration parameters such as renew-timer etc. This is normally handled by the !SimpleParser4 and !SimpleParser6 which seed the configuration structure with the default values and then drive them to all subnets. With the commands it is different, because the command merely contains subnet configuration and no global parameters. The global parameters have to be learned from the server and that poses a problem because they are again held in the SimpleParsers within the DHCPv4/v6 code, not in the library that the hook can access. So, I also had to move the SimpleParsers to a common place.

So, you're going to find a lot of code moved and that makes the code diff so huge. Also, if you see any minor issues in the moved code, I'd suggest to not focus on that in the review because it is just a moved piece of code.

Finally, we had intended to add 'leases-action' and 'reservations-action' into the 'subnetX-de'l commands. The problem is that our lease/hosts API doesn't have functions like "delete all leases/reservations for a given subnet id". Until this is supported we really not have means to address this in the hook library. This is a whole new thing to implement.

The User's Guide has been updated with the information about the new hooks library as well as with the data structures that constitute commands and responses.

Proposed ChangeLog entry for hook library:

XX.	[func]		marcin
	subnet_cmds library now supports commands for adding and deleting
	subnets from the current configuration.
	(Trac #5315, git abcd)

and for the main repo:

XXXX.	[doc]		marcin
	Documented subnet_cmds library in the Kea User's Guide.
	(Trac #5315, git abcd)

comment:3 Changed 2 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:4 follow-up: Changed 2 years ago by tmark


Kea-Repo
I updated guide/hooks.xml with some minor wording changes and fixed the
<section>ing, please pull first.


There was a naming conflict between 5314 and 5280, they both added SubnetIdIndexTag?.
5314 added it to dhcpsrv/subnet.h and 5280 added it to dhcpsrv/memfile_lease_storage.h.
When merging 5314, I renamed the former to SubnetSubnetIdIndexTag?. That change will
need to be done here too.


Q: Why did we not implement subnetX-update? I can completely see people
wanting to update options etc, without having to delete and the
add back the subnet. I know the requirements say "may" for these but
given that people have to pay for this, I think it a bit of a shortcoming
I don't see any tickets for this. At the very least, we should add a
ticket to cover it.

In the admin guide:

The subnetX-add sections depict the response as containing a list "subnetX", but the code
creates a list called "subnet-ids".


Premium-Repo

Q: When adding a subnet, what happens if I specify an ID of 0 or omit it?
Does it automatically assign one? --- if not that's a hole. The hooks guide
should describe this, whichever it is.


It's a little odd that arguments for both command/response for subnetX-adds
are lists, while for deletes the command argument is a single value (an id).
Are we thinking we might want to be able to add more than one at a time?
If not, why not delete more than one at time? Why doesn't delete return a
list of the id/prefix deleted like add does?


subnetX-del:
Treats subnet not found as an error. They should return CONTROL_RESULT_EMPTY
to be consistent with the "get" and lease commands .


It is confusing that subnet ID is sometimes an element called "subnet-id"
and sometimes it is an element named "id". The latter is from our configuration
syntax. Users may find this annoying.

comment:5 Changed 2 years ago by tmark

  • Owner changed from tmark to Unassigned

comment:6 Changed 2 years ago by tomek

  • Owner changed from Unassigned to tomek

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

  • Add Hours to Ticket changed from 30 to 5
  • Owner changed from tomek to tmark
  • Total Hours changed from 30 to 35

Virtual Marcin reporting here.

Thanks for reviewing this work. This branch was based on code that was old, so the first thing was to rebase it on current
master. When reviewing the changes, please see trac5315_rebase. (this applies to kea repo, the premium repo stays on trac5315).

Replying to tmark:


Kea-Repo
I updated guide/hooks.xml with some minor wording changes and fixed the
<section>ing, please pull first.

Thanks. The merge of that file was really messy. Git tried to combine leases documentation and subnets documentation.


There was a naming conflict between 5314 and 5280, they both added SubnetIdIndexTag?.
5314 added it to dhcpsrv/subnet.h and 5280 added it to dhcpsrv/memfile_lease_storage.h.
When merging 5314, I renamed the former to SubnetSubnetIdIndexTag?. That change will
need to be done here too.

I did as you suggested.


Q: Why did we not implement subnetX-update? I can completely see people
wanting to update options etc, without having to delete and the
add back the subnet. I know the requirements say "may" for these but
given that people have to pay for this, I think it a bit of a shortcoming
I don't see any tickets for this. At the very least, we should add a
ticket to cover it.

That's a mighty good suggestion. Created #5348.

In the admin guide:

The subnetX-add sections depict the response as containing a list "subnetX", but the code
creates a list called "subnet-ids".

As you pointed out, the use was inconsistent across calls. I have updated many of them and in general all calls now return subnets, which contain the details of the subnet being affected.


Premium-Repo

Q: When adding a subnet, what happens if I specify an ID of 0 or omit it?
Does it automatically assign one? --- if not that's a hole. The hooks guide
should describe this, whichever it is.

Good question. When subnet-id is not specified, we use our auto-increment mechanism. It's very simple - just pick the last auto-generated id and increase it by one. If you use it carefully, it will work. If you do stupid tricks, like specify a non-consecutive subnet-id and then rely on auto-generator, you will regret it. This autogeneration mechanism should be improved one day, but until then, we'll have to live with the new text I added. It tries to warn people to not do stupid things.


It's a little odd that arguments for both command/response for subnetX-adds
are lists, while for deletes the command argument is a single value (an id).
Are we thinking we might want to be able to add more than one at a time?

No, because of error handling. If you add one, the outcome is very definitive. It was either a success or a failure. With multiple subnets being added it would be blurry what the status to return and people would assume different things.

If not, why not delete more than one at time? Why doesn't delete return a
list of the id/prefix deleted like add does?

It does now. I don't think we'll ever allow the case of adding or deleting multiple subnets, but at least we're consistent among all calls. Also, the data structure is extensible if anyone ever comes to us and requests the ability to do multiple subnets in one shot.


subnetX-del:
Treats subnet not found as an error. They should return CONTROL_RESULT_EMPTY
to be consistent with the "get" and lease commands .

Updated. Also changed some unit-tests to cover that new approach.


It is confusing that subnet ID is sometimes an element called "subnet-id"
and sometimes it is an element named "id". The latter is from our configuration
syntax. Users may find this annoying.

They're now called "id" in every response. I ran out of time for converting the input, so there may be places where it still expected "subnet-id". Feel free to convert it or wait for either Marcin or myself to return.

Also, I created #5347 to cover lease deletion from specific subnet. We'll discuss this on the next Kea call how important it seems to people. Personally, I'd say it's something we could do after 1.3 beta, but at the same time I acknowledge that it may look more like a feature than a bugfix to some people.

comment:8 Changed 2 years ago by tmark

  • Add Hours to Ticket changed from 5 to 2
  • Total Hours changed from 35 to 37

The changes are fine.

premium:
I went ahead and did the input side conversion, so it is "id" for both inbound and outbound for subnet commands.

kea:
Updated subnet command texts accordingly in doc/guide/hooks.xml.

comment:9 Changed 2 years ago by tmark

  • Add Hours to Ticket changed from 2 to 4
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 37 to 41

Merged kea changes under git d259f330a1690b20eb368f6252f5da2cdb6187de and dded ChangeLog? 1288 for trac 5315.

Merged premium changes under git 2e09d82696679084c84a61fde622adc379adfcd7 and added ChangeLog? entry 11

Ticket is complete.

comment:10 Changed 2 years ago by vicky

  • Milestone changed from Kea1.3 to Kea1.3 beta

Milestone renamed

Note: See TracTickets for help on using tickets.