Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#3358 closed defect (fixed)

Default dhcp-ddns config does not have all elements

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

Description (last modified by tmark)

Follow on to #3339, the default dhcp-ddns configuration element in
Kea4 and Kea6 only contains the enables-update element. This makes
updating individual elements not function.

Subtickets

Change History (14)

comment:1 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 0 to 8
  • Owner set to UnAssigned
  • Status changed from new to reviewing

The PROBLEM:

Upon initial creation of a Kea component (i.e. b10-dhcp4 or b10-dhcp6), the
call to getFullConfig() within the configuration handler would return the
"dhcp-ddns" portion of server's configuration populated with all of its
items' default values. This is the expected behavior.

If a dhcp-ddns item value was changed (via bindctl), such as "server-port" or
"enable-updates", this method was correctly returning "dhcp-ddns" with all of
the default values MERGED with the changed value(s). This is also the desired
behavior.

Once the server is restarted, however, the "dhcp-ddns" element is returned
containing only the changed values. This causes parsing to fall over with
missing values that should have been supplied via item defaults.

The underlying problem is in config::ConfigData::getFullConfig(). This method
was doing a flat replacement of top level maps with their configured values,
not a merge of both default and configured values.

The SOLUTION:

What was done to address the problem was to modify ConfigData::getFullConfig
to call a new method, getFullValue(), to properly populate top level map
items with a merge of default and configured values.

When defining maps in spec files, each item can be given a default OR
specified as part of the map's default value. The getFullValue() method use
only one of these two sources. If the map default value is not empty
(i.e. not "{}"), then it will use the values specified here. Otherwise, it
use the default value specified per map item:

Note that a maps within maps, will not not work correctly without further
modifications to make getFullValue recursive.

ChangeLog? recommendation:

7xx.    [bug]      tmark
    Configuration session mechanisms now return top level map items as a
    merge of their default values with any values that have been changed.
    This corrects a bug in which the DHCP servers (b10-dhcp4 and b10-dhpc6)
    would fail configuration parsing during start-up after one or more
    of the values in their "dhcp-ddns" configuration section were 
    changed via bindctl.
    (Trac #3358, git TBD)

comment:2 Changed 4 years ago by tmark

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

comment:3 Changed 4 years ago by tmark

Fix does not work correctly. If works for one round of changes only.
If you change dhcp-ddns.server-port and save it that works.

If you then change some other value, dhcp-ddns.prefix for example and save that, it
reverst server-port to default value.

comment:4 Changed 4 years ago by tmark

  • Description modified (diff)
  • Feature Depending on Ticket set to 3373
  • Status changed from accepted to reviewing

The issue cited above is due to a problem in the python configuration library, that is
being fixed under Trac #3373. The changes made under this ticket work as it intended.

comment:5 Changed 4 years ago by tmark

  • Owner changed from tmark to UnAssigned

comment:6 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 8 to 2
  • Total Hours changed from 0 to 10

comment:7 Changed 4 years ago by marcin

  • Owner changed from UnAssigned to marcin

comment:8 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 2 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 10 to 11

Reviewed commit 72f017e69f32c908212ff91d8305b32cd9dcff45

src/lib/config/config_data.h
Typos in getFullValue description:

    /// If the item referredt toyidentifier is not a map item, then it simply
    /// returns the value yielded by getValue() .  If the identifier
    /// refers to a map item, then it returns a merged map of the map
    /// items' defaults and configured values.

I don't understand why the boolean parameter is passed by reference but it seems that you just stick to a general convention in this file with respect to this (also doxygen comments), so it is ok.

The spec43.spec must be included in the src/lib/config/tests/testdata/Makefile.am. Otherwise you will have issues with distcheck....

Running test....

There you go:

[ RUN      ] ConfigData.getFullConfigMapLevelDefault
unknown file: Failure
C++ exception with description "Error opening /home/marcin/devel/bind10/bind10-20140306/_build/../src/lib/config/tests/testdata/spec43.spec: No such file or directory" thrown in the test body.
[  FAILED  ] ConfigData.getFullConfigMapLevelDefault (0 ms)

Once you fix it, please merge.

comment:9 Changed 4 years ago by tmark

The changes made thus far have been discarded. Tickets #3339 and 3373 had to be reverted as they broke component removals (see #3374) and likely other things. At this point in time, whether or not we will even continue to use configuration libraries is up for discussion.

The new solution for this ticket is to simply add support for supplying default values into the D2ClientConfigParser. This allows the user to modify the dhcp-ddns values for
Kea servers by only supplying the values that are not defaults.

There is still a fundamental issue with map-items where by sequential changes to their
items via bindctl are not cummulative.

Without this ticket, the user would have to specify all the items in dhcp-ddns as part of a single "config commit", with this ticket they specify only the items that are
not default values. "enable-updates" is always required.

To enable updates with prefix of "newhost" and suffix of "bubbles.com.", and overriding
client updates you would submit this:

> config set Dhcp6/dhcp-ddns/enable-updates true
> config set Dhcp6/dhcp-ddns/generated-prefix "newhost"
> config set Dhcp6/dhcp-ddns/qualifying-suffix "bubbles.com."
> config set Dhcp6/dhcp-ddns/override-client-update true
> config commit 

If you then decided to turn off the override you would submit this:

> config set Dhcp6/dhcp-ddns/enable-updates true
> config set Dhcp6/dhcp-ddns/generated-prefix "newhost"
> config set Dhcp6/dhcp-ddns/qualifying-suffix "bubbles.com."
> config commit 
Last edited 4 years ago by tmark (previous) (diff)

comment:10 Changed 4 years ago by tmark

  • Add Hours to Ticket changed from 1 to 4
  • Feature Depending on Ticket 3373 deleted
  • Owner changed from tmark to marcin
  • Total Hours changed from 11 to 15

comment:11 follow-up: Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 4 to 1
  • Owner changed from marcin to tmark
  • Total Hours changed from 15 to 16

Reviewed commit a8560aefbfaffd86f4c63b500c38ee4a1bd8f8de

In the dhcp4.spec there is this:

            {

                "item_name": "always-include-fqdn",
                "item_type": "boolean",
                "item_optional": true,
                "item_default": false,
                "item_description": "Enable always including the FQDN option in its response"
            },

but there is no corresponding constant specifying a default.

In fact, I just saw that the false value is explicitly specified as ''false'' in the build function. Maybe you intended to specify a constant for this default value too?

comment:12 in reply to: ↑ 11 Changed 4 years ago by tmark

Replying to marcin:

Reviewed commit a8560aefbfaffd86f4c63b500c38ee4a1bd8f8de

In the dhcp4.spec there is this:

            {

                "item_name": "always-include-fqdn",
                "item_type": "boolean",
                "item_optional": true,
                "item_default": false,
                "item_description": "Enable always including the FQDN option in its response"
            },

but there is no corresponding constant specifying a default.

In fact, I just saw that the false value is explicitly specified as ''false'' in the build function. Maybe you intended to specify a constant for this default value too?

This option is actual supposed to be removed under #3294, however for completeness sake I have added a constant for it. Also, the ChangeLog entry will now be:

7xx.    [bug]      tmark
    Configuration parsing in b10-dchp6 and b10-dhcp4 for the "dhcp-ddns"
    section of their configurations now supplies hard-coded default values
    rather than those from their spec files.  This is a temporary solution
    to circumvent an issue in the configuration libraries which causes
    map-items to behave incorrectly.
    (Trac #3358, git TBD)

comment:13 Changed 4 years ago by tmark

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 16 to 17

Changes merged with 983d8acec3a7ccb1ffef662eac7518aed5f99381
Added ChangeLog entry 771.

comment:14 Changed 3 years ago by hschempf

  • Milestone changed from Kea1.0 to Kea0.9
  • Version set to git
Note: See TracTickets for help on using tickets.