Opened 3 years ago

Closed 3 years ago

#5132 closed enhancement (complete)

Host Reservation by a custom identifier

Reported by: tomek Owned by: tomek
Priority: medium Milestone: Kea1.2
Component: host-reservations 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: 1
Total Hours: 25 Internal?: no

Description

A user came up with a request to have host reservations based on both remote-id and circuit-id. We support reservation by circuit-id, but not by remote-id. But adding remote-id support is not enough. He explicitly said he need reservation by a combination of both identifiers.

My proposal is to have a custom identifier. First, the user would specify an expression in a similar way to how we define client classification today, e.g. "relay[*].option[37]+relay[*].option[45]". He would then add reservations with identifier-type set to custom and the identifier-value set to whatever he expects the expression to be.

On a related note, we need a hook for selecting a reservation.

Subtickets

Change History (21)

comment:1 Changed 3 years ago by tomek

This seems like a worthy feature for 1.3, also possibly a premium content.

comment:2 Changed 3 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.3

Per Kea meeting Feb 16, move to 1.3

comment:3 Changed 3 years ago by hschempf

  • Milestone changed from Kea1.3 to Kea1.2

Per Tomek, to be included in 1.2. Changing ticket milestone accordingly.

comment:4 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 0 to 20
  • Owner set to Unassigned
  • Status changed from new to reviewing
  • Total Hours changed from 0 to 20

The code is now ready for review. Note the changes are available on trac5132 branches in both kea and premium directories. Please refer to KeaPremium? on our internal wiki to find out how to check out both repos.

12XX.	[func]		tomek
	Flexible Identifier (flex-id) hook library has been developed. It
	allows users to specify a custom expression that takes any
	option, field, characteristic or property of the packet to be
	used as identifier and then do reservations based on the evaluated
	expression for each incoming packet.
	(Trac #5132, git tbd)

One outstanding thing that I wanted to keep out of this ticket is SQL schema update. This is something we can do as independent ticket.

comment:5 Changed 3 years ago by fdupont

  • Owner changed from Unassigned to fdupont

comment:6 Changed 3 years ago by fdupont

As far as I know we don't have a Jenkins VM ready for premium stuff (I suggest to open a QA ticket for this) so I am compiling on my macOS iMac at home...
I am not very happy with the flex_id directory name but it is the way of life (so don't change it without a (more) serious reason.

comment:7 Changed 3 years ago by fdupont

Fixed some spelling errors (e.g. the doc change in hooks.xml is not about Flexible Indentifier...). BTW I suggest to make hooks.xml (and other .xml files) tab free.

Compiling was good (BTW I got register warnings on lexer files: this means you used an old version of flex or forgot to regenerate src/lib/eval flex/bison, configure didn't pass the silent rule down too (I am afraid it will be hard to fix but as it is just not very convenient I suggest to just give up)).

make check is good too (same comment about configure vs submodule. BTW you managed to get it working: I remember a similar ticket for legal log unit tests).

continuing with (real) review.

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

doc/guide/hooks.xml: there are sometimes scenario -> scenarios (I'd prefer the real Italian plural but Stephen can confirm we should not use it).
Note I did not fix it leaving the choice...

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

  • Owner changed from fdupont to tomek

minor: dhcp[46]_hooks.dox say flex_id hook library is needed when docs say required. Note I prefer required.

Perhaps we should add an unit test where the hook returns one of the previous identifiers?

In Dhcpv[46]SrvTest please add a space between hook_index_host[46]_identifier and = even if this means to reinvent the whole block.

finished src/bin: code seems sane.

I can't find why CfgHosts::toElement6() was not updated. IMHO it was simply forgotten (but fortunately trivial to fix).
As I found what I think is a real issue I give the ticket back to you but continue the review.

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

Missing {{break}} in Host::getIdentifierAsText:

    case IDENT_FLEX:
        s << "flex-id";
// << here
    default:

IMHO the LAST_IDENTIFIER_TYPE in host.h must be updated:

    /// @brief Constant pointing to the last identifier of the
    /// @ref IdentifierType enumeration.
    static const IdentifierType LAST_IDENTIFIER_TYPE = IDENT_CLIENT_ID;

Note there is a possible comment to update at the beginning of pgsql_host_data_source.h.

Something I have no immediate answer: should the auto keyword includes flex-id? Even there is a clear answer I strongly suggest to update the auto ticket.

IMHO HostTest::getIdentifier unit test should be updated (and the spelling error on unuspported fixed).

Unfortunately it is impossible to check if evaluateString() really gets a string (as all values are represented as a string). BUT as the parser itself can be statically typed we can say it is not possible to not get a string. So I am very happy to have insisted to use typed parsers (and you to have been convinced)...

Note this is the reason I do not understand why string_expressionis not string_expr in place of bool_expr | string_expr. BTW the doc is clear about the parser to return a string! true is not a string, only the string representation of the true boolean.

The testExpressionString in evaluate_unittest.cc must check that neither "true" or "false" are returned.

There must be a few new unit tests showing that evaluateString() fails on not string (so boolean) expressions.

Finished the kea part, going to the premium part.

comment:11 Changed 3 years ago by fdupont

To come back to the issue of typing the output of a parser, you have 3 solutions:

  • give up and pray users doing silly things won't complain
  • tag values with a type, i.e. in our case extend the value type and add many checks to dynamically enforce consistency
  • statically type parser rules so consistency can be verify once (once means when writing rules).

We did the last even it made rules more complex but now we paid this price so please take the benefit...

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

Now the premium code...

I pushed a few spelling fix (e.g paramters)

Doxyfile is an 1.8.6 when the master has a 1.8.11 (not only it is obsolete but a diff does not work).
I retried with the legal log one (which BTW needs to be upgraded) and the PROJECT_LOGO paths are different. Note it seems the bug is in legal_log... in both cases make devel worked and I can't say using a remote box.

In flex_id/Makefile.am please change @PACKAGE@ by @TOP_PACKAGE@ (it is a legal_log fix so I am not surprised it was not ported).

It is minor but libdhcp_flex_id.so depends on asiolink, dns++, cryptolink and cc too.

License are different too but there is a ticket saying current legal log license is incorrect so I suppose you have the right one. BTW it uses too long lines (please fix: we should not play with legal things).

In callout.cc std::memcpy(&id[0], &value[0], value.size()); must not be called with empty arguments.

In flex_id.h missing space in namespace isc{
`
In flex_id_log.cc there is still references to legal logging... I suggest a grep -r legal as there are in some other files.

IMHO in the .dox the "This documentation is addressed at Kea developers' does not really apply?

For Stephen calculated -> computed?

I have a concern with caps in One string Hook Library Parameters.

Still about the .dox file: it finishes by but rather to so I think there are some missing words.

In load_unload.cc @return Returns -> @return (and BTW it is a legal file todo fix case too)

data::ConstElementPtr -> ConstElementPtr (both because isc::data is declared and because partial namespaces are a bad practice)

RotatingFile in unload() comment

Same comment about missing indirect dependencies.

callout_unittests.cc file has a 2016-2017: I don't know what is the rule when a new file was copied and adapted from an old one?

Spurious //#Include <test_utils.h> (note the uppercase I)

Note I can use vector<uint8_t> expected = { 0, 0, 0, 1 }; but I set the CXX environment variable too.

To summary there are some minor issues in the premium code but nothing blocking.

BTW I have a question: does it really work, i.e., was the host multi map flexible enough? (I didn't check yet and the last type I looked at it the host multi map was a bit hairy). PS: git the answer: the multi map uses access method for the id and its type so it work well without extra work.

You can fix or ask me to fix these minor concerns (if I am fixing this we should agree about the exact way to wrap the license statement lines).

I don't give the ticket to you as I already did this...

Last edited 3 years ago by fdupont (previous) (diff)

comment:13 in reply to: ↑ 8 Changed 3 years ago by tomek

Replying to fdupont:

doc/guide/hooks.xml: there are sometimes scenario -> scenarios (I'd prefer the real Italian plural but Stephen can confirm we should not use it).
Note I did not fix it leaving the choice...

Corrected as suggested.

comment:14 in reply to: ↑ 9 Changed 3 years ago by tomek

Replying to fdupont:

minor: dhcp[46]_hooks.dox say flex_id hook library is needed when docs say required. Note I prefer required.

It says 'required' in both files now.

Perhaps we should add an unit test where the hook returns one of the previous identifiers?

Unit-test added. It returns hwaddress.

In Dhcpv[46]SrvTest please add a space between hook_index_host[46]_identifier and = even if this means to reinvent the whole block.

Added.

I can't find why CfgHosts::toElement6() was not updated. IMHO it was simply forgotten (but fortunately trivial to fix).

Oops. It's updated now.

comment:15 in reply to: ↑ 10 Changed 3 years ago by tomek

Replying to fdupont:

Missing {{break}} in Host::getIdentifierAsText:

    case IDENT_FLEX:
        s << "flex-id";
// << here
    default:

Fixed.

IMHO the LAST_IDENTIFIER_TYPE in host.h must be updated:

    /// @brief Constant pointing to the last identifier of the
    /// @ref IdentifierType enumeration.
    static const IdentifierType LAST_IDENTIFIER_TYPE = IDENT_CLIENT_ID;

Updated (the constant and the unit-tests that failed after that change).

Note there is a possible comment to update at the beginning of pgsql_host_data_source.h.

I left DB schema updates out of this work. There's #5195 added to 1.2-final.

Something I have no immediate answer: should the auto keyword includes flex-id? Even there is a clear answer I strongly suggest to update the auto ticket.

In my opinion the whole 'auto' concept is not worth the effort. There's simply no way to make the corner cases work. It was nice concept, but once we started storing reservations in DB, we never know what kind of reservations were just inserted. Anyway, updated #5103 with the note that flexible identifier was added.

IMHO HostTest::getIdentifier unit test should be updated (and the spelling error on unuspported fixed).

Updated.

Note this is the reason I do not understand why string_expressionis not string_expr in place of bool_expr | string_expr. BTW the doc is clear about the parser to return a string! true is not a string, only the string representation of the true boolean.

Updated the expression. It's now string_expr only.

The testExpressionString in evaluate_unittest.cc must check that neither "true" or "false" are returned.

No, for two reasons. First, the rule has been updated to not accept booleans as strings anymore. Second, I can imagine case where user wants to get value of an option and that option could contain "true" (as a 4 bytes long string). That's very unlikely, but still valid case.

There must be a few new unit tests showing that evaluateString() fails on not string (so boolean) expressions.

Updated the test code. It now tests couple bool expressions and checks that string parser rejects them.

Finished the kea part, going to the premium part.

Thanks, moving on to premium fixes.

comment:16 Changed 3 years ago by fdupont

Note as premium fixes are very minor I am checking again the main code and if it is right (I can't see how it could not be) you may merge and close this.
BTW please update #5183 so we can address it too.

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

  • Add Hours to Ticket changed from 20 to 4
  • Owner changed from tomek to fdupont
  • Total Hours changed from 20 to 24

Replying to fdupont:

Now the premium code...

I pushed a few spelling fix (e.g paramters)

Merci.

Doxyfile is an 1.8.6 when the master has a 1.8.11 (not only it is obsolete but a diff does not work).
I retried with the legal log one (which BTW needs to be upgraded) and the PROJECT_LOGO paths are different. Note it seems the bug is in legal_log... in both cases make devel worked and I can't say using a remote box.

Upgraded both legal_log and flex_id Doxyfiles to 1.8.11 and corrected paths, so logo is now displayed correctly.

In flex_id/Makefile.am please change @PACKAGE@ by @TOP_PACKAGE@ (it is a legal_log fix so I am not surprised it was not ported).

That didn't work. I @PACKAGE@ gets replaced by kea-premium directory, but @TOP_PACKAGE@ remains copied verbatim, so I ended up having something like /var/@TOP_PACKAGE@ in my Makefile. This is on Ubuntu 16.04 with automake 1.15, autoconf 2.69. So I left this as is. If you know how to fix it, please do so. I you don't know right away, I'd say we merge the code and fix it in 1.2-final.

It is minor but libdhcp_flex_id.so depends on asiolink, dns++, cryptolink and cc too.

Added asiolink, dns++ and cc, but I don't know why there's dependency on cryptolink?

License are different too but there is a ticket saying current legal log license is incorrect so I suppose you have the right one. BTW it uses too long lines (please fix: we should not play with legal things).

Fixed the long lines problem for flex_id. I left legal_log untouched.

In callout.cc std::memcpy(&id[0], &value[0], value.size()); must not be called with empty arguments.

Added appropriate if.

In flex_id.h missing space in namespace isc{

Added.

In flex_id_log.cc there is still references to legal logging... I suggest a grep -r legal as there are in some other files.

Removed all references (I hope I haven't missed any).

IMHO in the .dox the "This documentation is addressed at Kea developers' does not really apply?

No, it doesn't. Updated for both legal_log and flex_id.

For Stephen calculated -> computed?

Updated.

I have a concern with caps in One string Hook Library Parameters.
Still about the .dox file: it finishes by but rather to so I think there are some missing words.

Uh oh. Forgot to update this one. Please pull and review. There's now a short section that explains how the library is working.

In load_unload.cc @return Returns -> @return (and BTW it is a legal file todo fix case too)

Fixed in both.

data::ConstElementPtr -> ConstElementPtr (both because isc::data is declared and because partial namespaces are a bad practice)

Fixed in both.

RotatingFile in unload() comment
Same comment about missing indirect dependencies.

I do not know how to check the indirect dependencies. If you think we need to add them, please do. But I hope we will fix the dependencies soon and then there won't be any need for including completely unrelated dependencies (like cryptolink or dns++).

callout_unittests.cc file has a 2016-2017: I don't know what is the rule when a new file was copied and adapted from an old one?

It's a new file, so should be 2017.

Spurious //#Include <test_utils.h> (note the uppercase I)

Removed.

Note I can use vector<uint8_t> expected = { 0, 0, 0, 1 }; but I set the CXX environment variable too.

premium makefiles are not updated to take advantage of C++11, so kept the file as is.

BTW I have a question: does it really work, i.e., was the host multi map flexible enough? (I didn't check yet and the last type I looked at it the host multi map was a bit hairy). PS: git the answer: the multi map uses access method for the id and its type so it work well without extra work.

I have not tested it myself, but that's something I plan to do after the code freeze (won't have time before that).

Thanks for you review.

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

Replying to tomek:

In flex_id/Makefile.am please change @PACKAGE@ by @TOP_PACKAGE@ (it is a legal_log fix so I am not surprised it was not ported).

That didn't work. I @PACKAGE@ gets replaced by kea-premium directory, but @TOP_PACKAGE@ remains copied verbatim, so I ended up having something like /var/@TOP_PACKAGE@ in my Makefile. This is on Ubuntu 16.04 with automake 1.15, autoconf 2.69. So I left this as is. If you know how to fix it, please do so. I you don't know right away, I'd say we merge the code and fix it in 1.2-final.

=> This fix is trac5080 which was merged between the start of trac5132 and now. So it works but only after a rebase or (easier) application of trac5080/trac5064 changes.

Fixed the long lines problem for flex_id. I left legal_log untouched.

About dependencies:

  • cryptolink comes form dns++ (TSIG code)
  • the way to get them is to use ldd (or otool -L for macOS) on binaries
  • indirect dependencies are not required (it works without them) but the style guideline highly recommends them for corner cases (i.e., strange cases where the link order matters). IMHO this is like spelling, some checker warnings, etc, it must be done from time to time, for instance by a 1.2-final.

I am reviewing the whole... If I don't post something in the next 30 mn the branches should be good for merge.

comment:19 Changed 3 years ago by fdupont

Note I applied the trac5080 fix so please pull the premium trac5132.

comment:20 Changed 3 years ago by fdupont

  • Owner changed from fdupont to tomek

I fixed a spelling error in each branch so please pull. I take the occasion to address #5183 too.
I built and passed tests on my macOS so now it is ready!

comment:21 Changed 3 years ago by tomek

  • Add Hours to Ticket changed from 4 to 1
  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 24 to 25

Thanks a lot for your review and fixes.

Code merged and verified on Ubuntu 16.04 x64.

Merged, pushed, closing ticket.

Note: See TracTickets for help on using tickets.