Opened 3 years ago

Closed 3 years ago

#5145 closed enhancement (fixed)

Put hooks libraries state into configuration (Cfg structure)

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.2
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets: #5114
Sensitive: no Defect Severity: N/A
Sub-Project: Mozilla Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 4 Internal?: no

Description (last modified by fdupont)

Address D2 client config mess too.

Subtickets

Change History (21)

comment:1 Changed 3 years ago by fdupont

  • Parent Tickets set to 5114

comment:2 Changed 3 years ago by fdupont

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

comment:3 Changed 3 years ago by fdupont

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

Ready for review.

comment:4 Changed 3 years ago by tomek

  • Description modified (diff)
  • Milestone changed from Kea-proposed to Kea1.2
  • Sub-Project changed from DHCP to Mozilla
  • Type changed from defect to enhancement

comment:5 Changed 3 years ago by fdupont

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

comment:6 Changed 3 years ago by fdupont

I am sorry but the hook parser was moved to the hook library: this cannot work because config structures are in the dhcp server library.

comment:7 Changed 3 years ago by fdupont

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

I moved back the hook parser to src/lib/dhcpsrv/parsers/dhcp_parsers.* (*).
I changed the agent config structure to use a CfgHooksLibraries ().

(*) if you'd like to put the hook parser down in the lib hierarchy you have to move too some config stuff to for instance the cc library (make sense as SimpleParser is already there).

() access methods return reference and const reference, IMHO more efficient than shared pointers and BTW more in the C++ style.

Ready for review (branch trac5145a).

comment:8 follow-up: Changed 3 years ago by tomek

I'm currently busy with the kea-shell implementation, so can't review it right now. But I'm sceptical that moving the hook parser back to libdhcp is a good idea. This means that you will need to include libdhcpsrv (which is a huge library) in every component. In paricular, in D2 and CA. Is that right? If that is so, I'm against this move.

comment:9 in reply to: ↑ 8 Changed 3 years ago by fdupont

Replying to tomek:

I'm currently busy with the kea-shell implementation, so can't review it right now. But I'm sceptical that moving the hook parser back to libdhcp is a good idea.

=> it is at least arguable but the current design is to put all the configuration is libdhcpsrv.

This means that you will need to include libdhcpsrv (which is a huge library) in every component.

=> I believe by include you mean dynamically link with. Static link is another beast (we can discuss about if (and only if) you want).

In particular, in D2 and CA. Is that right?

=> it is right and false: the libdhcpsrv has to be linked but not because of the hooks library config structure. BTW I did *not* touch the src/bin/agent/Makefile.am file which has 18 kea_ctrl_agent_LDADD for src/lib so I did not make this worse.

if that is so, I'm against this move.

=> this is not a solution but as I agree we want to get a CA without big and not essentially related libraries I propose:

  • we stay with the hook parser in the libdhcpsrv
  • we shall create a ticket for Kea 1.3 to find all dependencies and if feasible to move code so CA will link only with a minimal set of libraries.

comment:10 Changed 3 years ago by fdupont

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

comment:11 Changed 3 years ago by fdupont

I rebased again the code:

  • hooks_parser.* is now (still) in src/lib/hooks
  • I put the configuration class (renamed into HooksConfig in src/lib/hooks/hooks_config.*
  • I moved the post-parse tools to this config class
  • as #1205 was merged I add an toElement method. You *must* adjust it to return or not an empty map as parameters when none was given (there is not a big difference and the unparse property is preserved by both options).
  • I did not added unit tests for toElement but I moved hooks after testutils in src/lib/Makefile.am
  • I did not moved the unit tests from src/lib/dhcpsrv/tests (IMHO only the fact they are somewhere is important).

Commit is 99705e8..cf8d112

I'll take benefit we have a fix before unparse ticket to address the D2 client mess.

comment:12 Changed 3 years ago by fdupont

  • Description modified (diff)

comment:13 follow-up: Changed 3 years ago by fdupont

I fixed the D2 client config mess:

  • removed the unused allow-client-update parameter
  • get rid of D2ClientConfig::isShortCutDisabled:
    • defaults are set unconditionally
    • qualifying-suffix (which has no default) must be set to be parse when updates are enabled

This allows a new way to shoot in your foot:

1- you create a config without D2 client
2- you get it by get-config/export-config
3- you edit the (full) config to enable updates
4- you get the constructor default (vs syntax default which does not exist) for qualifying-suffix

IMHO the best is to warn about this. BTW don't try to fix the constructor or D2ClientConfig code: it will be complex and it won't work as many tests use the default constructor and the next line enableUpdates(true)...

Commit f15dee70541f385ce2ce0cd376c190382439c73a

IMHO the database parser should be rewrote too:

  • reorganize lease, host and shared code
  • avoid to and from string representation changes
  • don't become insane on {{{

{ "lease-database": { "name": "/tmp/bug my=dear" } }
}}}
but we should discuss about what we exactly want first/before...

comment:14 Changed 3 years ago by fdupont

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

Ready for review. Branch is now trac5145b (and please read ticket comments).

comment:15 Changed 3 years ago by tomek

  • Owner changed from UnAssigned to tomek

comment:16 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by tomek

Replying to fdupont:

I fixed the D2 client config mess:

  • removed the unused allow-client-update parameter
  • get rid of D2ClientConfig::isShortCutDisabled:
    • defaults are set unconditionally
    • qualifying-suffix (which has no default) must be set to be parse when updates are enabled

This allows a new way to shoot in your foot:

1- you create a config without D2 client
2- you get it by get-config/export-config
3- you edit the (full) config to enable updates
4- you get the constructor default (vs syntax default which does not exist) for qualifying-suffix

IMHO the best is to warn about this. BTW don't try to fix the constructor or D2ClientConfig code: it will be complex and it won't work as many tests use the default constructor and the next line enableUpdates(true)...


Commit f15dee70541f385ce2ce0cd376c190382439c73a

IMHO the database parser should be rewrote too:

  • reorganize lease, host and shared code
  • avoid to and from string representation changes
  • don't become insane on {{{

{ "lease-database": { "name": "/tmp/bug my=dear" } }
}}}
but we should discuss about what we exactly want first/before...

What is the exact problem with the current code? If there are things to improve there, but they don't prevent us from implementing toElement (to get get-config and write-config), we can postpone them. The best time to reorg them would be when/if we will be touching this area. That may happen in 1.3, but we haven't made any decisions yet. (Two possible reasons to touch this code in 1.3: adding host reservation support for Cassandra or adding support for keeping subnets in the database). Anyway, for now I think we can leave the code as is.

comment:17 follow-up: Changed 3 years ago by tomek

  • Owner changed from tomek to fdupont
  • Total Hours changed from 0 to 4

I have reviewed all changes on trac5145b. Thanks a lot for rebasing this, it's very useful for review (and also makes the merge easier).

Did couple minor corrections. Please pull and review.

hook_parser.h
Libraries parameter of the parse() method was not documented.
Added missing comment. Please pull.

srv_config.cc
The code in SrvConfig::equals that compares two hooklibcollections belongs to
HooksConfig?. I did move the code. Please pull and review.

Having said that, I think the code in the HooksConfig::equal does not compare
the two list of libraries correctly. It assumes that the libraries order does
not matter, but it does matter. The libraries are loaded and its callouts are
called in order they're loaded, so changing the libraries order may change the
server behavior. Thus the order is important.

But I agree this is extremely small window of this making any difference, so we
can keep the code as is for now. I added the comment explaining this problem
to HooksConfig::equal.

srv_config_unittest.cc
The SrvConfigTest?.hooksLibraries checked if the copied config has 2 library
entries, but did not check if these were the right entries. Added missing check.

*.hh files src/bin/dhcp{4,6}
The changes in those files were only changing the timestamp. I typically omit
those file whet commiting changes. But it's ok to include them, I suppose.

The code builds and unit-tests pass on Ubuntu 16.04.1 x64.

The change regarding removal of allow-client-update requires a ChangeLog?. If you
don't have any better proposal, here's mine:

12XX.	[func]		fdupont
	Obsolete parameter 'allow-client-update' has been removed
	from DHCPv4 and DHCPv6 components.
	(Trac #5145, git tbd)

comment:18 in reply to: ↑ 16 Changed 3 years ago by fdupont

Replying to tomek:

IMHO the database parser should be rewrote too:

  • reorganize lease, host and shared code
  • avoid to and from string representation changes
  • don't become insane on {{{

{ "lease-database": { "name": "/tmp/bug my=dear" } }
}}}
but we should discuss about what we exactly want first/before...

What is the exact problem with the current code?

=> mainly the second point.

If there are things to improve there, but they don't prevent us from implementing toElement (to get get-config and write-config), we can postpone them.

=> it does not prevent us from implementing toElement.

The best time to reorg them would be when/if we will be touching this area. That may happen in 1.3, but we haven't made any decisions yet. (Two possible reasons to touch this code in 1.3: adding host reservation support for Cassandra or adding support for keeping subnets in the database). Anyway, for now I think we can leave the code as is.

=> I conclude the same thing. It should be done one day but not soon.

comment:19 in reply to: ↑ 17 Changed 3 years ago by fdupont

Replying to tomek:

I have reviewed all changes on trac5145b. Thanks a lot for rebasing this, it's very useful for review (and also makes the merge easier).

Did couple minor corrections. Please pull and review.

Having said that, I think the code in the HooksConfig::equal does not compare
the two list of libraries correctly. It assumes that the libraries order does
not matter, but it does matter. The libraries are loaded and its callouts are
called in order they're loaded, so changing the libraries order may change the
server behavior. Thus the order is important.

=> I agree: in general the list order in configurations does not matter (i.e., lists
(array in JSON terms) are used as sets) but it is one of the rare cases where
it can be false.

*.hh files src/bin/dhcp{4,6}
The changes in those files were only changing the timestamp. I typically omit
those file whet commiting changes. But it's ok to include them, I suppose.

=> without a change git does not save the last modification date and make
does not know what to do.

The change regarding removal of allow-client-update requires a ChangeLog?.

=> yes, it is a visible change. I'll use you proposal.

comment:20 Changed 3 years ago by fdupont

Merged. Closing.

comment:21 Changed 3 years ago by fdupont

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.