#4265 closed enhancement (fixed)

Make options inserted by a v6 relay agent available for the classification

Reported by: tomek Owned by: sar
Priority: medium Milestone: Kea1.1
Component: classification Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 24 Add Hours to Ticket: 1
Total Hours: 46 Internal?: no

Description

One of the requirements of the design is to provide an ability to access v6 options inserted by a relay.

This ticket covers accessing options inserted by a DHCPv6 relay agent, i.e. accessing options stored on various levels of encapsulation in relay-forward message(s), possibly nested multiple times.

Subtickets

Change History (7)

comment:1 Changed 21 months ago by sar

I'm adding the work for link-addr and peer-addr here (from 4269) as they are per relay and should be handled in the same style as relay options.

comment:2 Changed 20 months ago by sar

  • Owner set to sar
  • Status changed from new to assigned

comment:3 Changed 20 months ago by sar

  • Owner changed from sar to unassigned
  • Status changed from assigned to reviewing
  • Total Hours changed from 0 to 40

Ready for review

My suggested change log entry is in the ChangeLog? file. It is:

1xxx.   [func]          sar
        Added access to the peer address, link address and option
        information added by relays in a DHCPv6 message.
        (Trac $4269, git tbd)

Some changes to pkt6.[cc h] were necessary to extract some of the information and to make the type signatures match.

One open issue is how to access information from the relay closest to the client. If there is only a single relay this is simple. If there can be more than one, and the number isn't known in advance, then we can't get to the one closest to the client. This can be fixed in another ticket by
1) Adding a named option in the nesting level (such as "first" or "last") which gets translated during evaluation to the proper nesting level.
2) Changing the code to put the nesting level on the value stack and allowing the user to access the total number of relays and choose the appropriate one.
3) Adding another token such as relay6any which would be similar to the get any relay features.

I think something like (1) is probably the best option (and 3 can be added with appropriate tags). As I'm not sure how necessary such features are I've deferred adding them in this ticket.

comment:4 follow-up: Changed 20 months ago by tmark

  • Add Hours to Ticket changed from 0 to 2
  • Owner changed from unassigned to sar
  • Total Hours changed from 40 to 42

I cleaned up some trailing whitespace stuff (showed up under git diff), so pull before you doing anything else.

All the unit tests pass unde OS-X.

src/lib/eval/eval_context.cc

uint8_t EvalContext::convertNestLevelNumber()

+ This can't happen...
+ error(loc, "Nest level has invalid value in " + nest_level);

I think you should remove the "this can't happen" comment. Sure, if this method is only after parsing as succesfully constructed the stack, then in theory we sshould have a valid integer string.

Also why do you not just boost::lexical_cast<uint8_t> to begin with?


src/lic/eva/token.cc

  • TokenRelay6Option::getOption(const Pkt& pkt)

Please expand the commentary for the isc::OutOfRange? catch to explain that
out-of-range equates to "not found" in this context. The level have already been validated so out-of-range here means that level of nesting isn't present in the packet.


src/lic/eva/token.h

Thanks for stating that nest_level 0 means closest to the server.


General

I think I would prefer TokenRelay6 be called TokenRelay6Field.


As to the open issue above, users are going to want a way to specify nest-level equal to the client without having to know the precise number and I think your first option is the the the way to go, where we have one or more special meaning constants. You should probably create a ticket to cover this.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 20 months ago by sar

  • Owner changed from sar to tmark

Replying to tmark:

I cleaned up some trailing whitespace stuff (showed up under git diff), so pull before you doing anything else.

Thanks

All the unit tests pass unde OS-X.

src/lib/eval/eval_context.cc

uint8_t EvalContext::convertNestLevelNumber()

+ This can't happen...
+ error(loc, "Nest level has invalid value in " + nest_level);

I think you should remove the "this can't happen" comment. Sure, if this method is only after parsing as succesfully constructed the stack, then in theory we sshould have a valid integer string.

Also why do you not just boost::lexical_cast<uint8_t> to begin with?

I was basically copying convertOptionCode, including the comment. I've removed the "this can't happen comment".

What happens when one passes a negative string for example "-1" to boost::lexical_cast<uint8_t>? With the current code it can be converted to a value and that can be compared to 0 and 31. Will casting it to a uint8_t cause an error to be thrown on the boost::lexical_cast call?


src/lic/eva/token.cc

  • TokenRelay6Option::getOption(const Pkt& pkt)

Please expand the commentary for the isc::OutOfRange? catch to explain that
out-of-range equates to "not found" in this context. The level have already been validated so out-of-range here means that level of nesting isn't present in the packet.

Done


src/lic/eva/token.h

Thanks for stating that nest_level 0 means closest to the server.


General

I think I would prefer TokenRelay6 be called TokenRelay6Field.

done


As to the open issue above, users are going to want a way to specify nest-level equal to the client without having to know the precise number and I think your first option is the the the way to go, where we have one or more special meaning constants. You should probably create a ticket to cover this.

I created 4482

comment:6 in reply to: ↑ 5 Changed 20 months ago by tmark

  • Add Hours to Ticket changed from 2 to 1
  • Estimated Difficulty changed from 0 to 24
  • Owner changed from tmark to sar
  • Total Hours changed from 42 to 43

Replying to sar:

Replying to tmark:

:

What happens when one passes a negative string for example "-1" to boost::lexical_cast<uint8_t>? With the current code it can be converted to a value and that can be compared to 0 and 31. Will casting it to a uint8_t cause an error to be thrown on the boost::lexical_cast call?

Yes, the lexical_cast will fail but that's sort of the whole point of using it in the first place. It will also fail for anything larger than 255. You could restructure it something like this which reduces the amount of code a bit:

uint8_t
EvalContext::convertNestLevelNumber(const std::string& nest_level,
                                    const isc::eval::location& loc)
{
    if (option_universe_ != Option::V6) {
        error(loc, "Nest level invalid for DHCPv4 packets");
    }

    uint8_t n = 0;
    try {
        n  = boost::lexical_cast<uint8_t>(nest_level);
        if (n < 32) {
            return(n);
        }
    } catch (const boost::bad_lexical_cast &) {
    }

    error(loc, "Nest level has invalid value in "
                      + nest_level + ". Allowed range: 0..31");
}

In the grand scheme of things it doesn't make a lot of difference, perhaps more stylistic than anything else. It just seems wasteful to do both the lexcial cast and then a static cast when you know from the outset what you need. I would have had the same comment for convertOptionCode() only I didn't review it. I don't think a user really needs a distinction between whether the value was wrong because it wasn't a number or whether it was wrong because it was a number whose value was invalid.

Also, we should probably define a maximum nest level constant defined in Pkt6. I realize this should have been done long before you did this work.

All the other changes are fine.


comment:7 Changed 20 months ago by sar

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Total Hours changed from 43 to 46

the lexical_cast using a <uint8_t> results in a character instead of an integer. So "0" turns into 0x30 instead of 0x00. After discussion on jabber I have left the code mostly as it was. I have changed from a hardcoded 31 to using the HOP_COUNT_LIMIT from the dhcp6.h file.

Note: See TracTickets for help on using tickets.