Opened 6 years ago

Closed 6 years ago

#3339 closed defect (fixed)

Merge config changes with existing config in Kea doesn't work for map elements

Reported by: tmark Owned by: tmark
Priority: high Milestone: Kea0.8
Component: ~dhcp-ddns(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: Core Feature Depending on Ticket:
Estimated Difficulty: 16 Add Hours to Ticket: 1
Total Hours: 3 Internal?: no

Description

When a configuration update is received by b10-dhcp4, logic in ControlledDhcpv4Srv::dhcp4ConfigHandler() merges the changed configuration elements sent by the control session into the full configuration before submitting it for configuration parsing.

However, the merge does not properly handle elements, which are themselves maps.

When a map element is altered through bindctl, the update message will contain the map element and only the sub-elements that were altered. For instance if you change "dhcp-ddns/server-port" to 413, the update message will contain this:

{ "dhcp-ddns": { "server-port": 413 } }

The merge logic ends up replacing the existing map element with the partial map element received rather than merging the two and using the result.

This causes the dhcp-ddns parser to invalidate the configuration due to missing parameters.

Either the merge logic needs to be augmented to correctly merge map elements or we need to rewrite all of the Kea parsing to permit processing updates.

In the meantime, if one wishes to alter one value in the mapped element you must respecify them all. In other words, if you want to alter one value in dhcp-ddns through bindctl, you must set all of them and then issue the commit.

Subtickets

Change History (7)

comment:1 Changed 6 years ago by tmark

  • Priority changed from medium to high

comment:2 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 0 to 2
  • Estimated Difficulty changed from 0 to 16
  • Owner set to tmark
  • Status changed from new to assigned
  • Sub-Project changed from DHCP to Core
  • Total Hours changed from 0 to 2

Analysis showed that the actual problem was in isc::data::merge(). I modified this function to call itself recursively to merge elements that are maps. Works like a champ now.

comment:3 Changed 6 years ago by tmark

  • Owner changed from tmark to UnAssigned
  • Status changed from assigned to reviewing

comment:4 Changed 6 years ago by tomek

  • Owner changed from UnAssigned to tmark

I tried to quickly look at that ticket, but there seem to be no commits on trac3339...

$ git pull
Already up-to-date.
$ git checkout trac3339
Switched to branch 'trac3339'
$ git diff $(git merge-base trac3339 master) trac3339 | wc -l
0

Hmmm.

comment:5 Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

Oops. Please try again.

comment:6 Changed 6 years ago by tomek

  • Owner changed from tomek to tmark

Reviewing ed75a8ddb78042a459132397a8232a49c3f4df92.

The code looks good. I have no specific comments, except that you
should provide a ChangeLog? as this is user visible fix.

Copyright for cc/tests/data_unittests.cc seems odd. It said 2009, but
git log shows that the first commit was done in 2010. Hmm. That's
just an observation, there's nothing to be done here.

It would be quickest to discuss the changelog on jabber and then
just merge it.

Code compiles on Ubuntu 13.10 x64 and all unit-tests pass.

comment:7 Changed 6 years ago by tmark

  • Add Hours to Ticket changed from 2 to 1
  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 2 to 3

Changes merged and created the following ChangeLog entry:

760.    [bug]       tmark
    When merging a map of configuration elements into another, elements that
    are themselves maps will be merged. In particular, this corrects a defect
    which caused a configuration commit error to occur when using bindctl to
    modify a single a parameter in dhcp-ddns portion of b10-dhcp4 configuration.
    (Trac# 3339, git 3ae0d93d89f3277a566eeb045191a43b2dd9d9b1)
                                                              
Note: See TracTickets for help on using tickets.