Opened 2 years ago

Closed 20 months ago

#4232 closed enhancement (complete)

Add new ip address literal for classification expression

Reported by: fdupont Owned by: fdupont
Priority: medium Milestone: Kea1.1
Component: classification Version: git
Keywords: classifcation 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: 0
Total Hours: 4 Internal?: no

Description

@<ip-address> for both IPv4 and IPv6 as a HexString? variant.

Subtickets

Change History (17)

comment:1 Changed 2 years ago by fdupont

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

comment:2 Changed 2 years ago by fdupont

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

Done (stacked under #4231).

comment:3 Changed 21 months ago by fdupont

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

comment:4 Changed 21 months ago by fdupont

Rebase, remove the front @.

comment:5 Changed 21 months ago by fdupont

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

Done, ready again for review.

comment:6 Changed 21 months ago by fdupont

Oops, please note the branch is trac4232a

comment:7 Changed 21 months ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

Per 3/3/ team meeting, accepted for 1.1. Noted that 4268 and similar tickets make sense only with this one. In review, so estimate being used = .5 days

comment:8 Changed 20 months ago by sar

  • Owner changed from UnAssigned to sar

comment:9 Changed 20 months ago by sar

  • Component changed from Unclassified to classification

comment:10 follow-up: Changed 20 months ago by sar

  • Owner changed from sar to fdupont
  • Total Hours changed from 0 to 2

I fixed a small typo, please do a pull before continuing.

Please ensure the copyrights are updated if necessary.

In the email from Feb 25 (and now added to the design document) there was a desire to not use an @ (or any other tag) to indicate an IP address. Either the code needs to be modified to remove the @ or the team needs to be convinced that using an @ is the better alternative.

There will need to be a change log entry and there will need to be an entry in the documentation describing the use of IP addresses.

The bulk of the code looks okay once the @ issue is addressed.

comment:11 in reply to: ↑ 10 Changed 20 months ago by fdupont

  • Owner changed from fdupont to sar

Argh! It seems you reviewed the wrong branch (i.e., trac4232 vs trac4232a).

comment:12 follow-up: Changed 20 months ago by sar

  • Owner changed from sar to fdupont
  • Total Hours changed from 2 to 4

Yes I reviewed the wrong version. Sorry.

doc/guide/classify.xml

The description should mention that the IP address can be v4, v6 or v4 mapped v6.

src/lib/eval/Makefile.am & src/lib/eval/tests/Makefile.am

I'm unclear as to why there are a number of new libraries being added (dns, crypto etc). Are these actually necessary?

src/lib/eval/lexer.ll

If I understand the patterns for addr4 and addr6 we are being permissive in what we will consider an IP address and then tossing strings that aren't addresses based on trying to create an IOAddress. This should be commented. I'm thinking of a line just before the convenience expressions for them.

src/lib/eval/tests/context_unittest.cc

There is a test for ::<something and :: there should be a test for <something>:: (the zeros at the end of the address)

src/lib/eval/tests/token_unittest.cc

The test is now for "ipaddress" and not "ipaddress6". It's name should probably be updated.

src/lib/eval/token.cc

In the constructor for TokenIpAddress? we simply ignore any throws from creating the IOAddress. I assume this is because we already validated the string as an address in lexer.ll. It would be useful to add a comment if that is the case.

It will need a Change Log entry. I suggest:
Added support for IP address (v4 and v6) literals in classification rules.

comment:13 in reply to: ↑ 12 Changed 20 months ago by fdupont

  • Owner changed from fdupont to stephen

Replying to sar:

Yes I reviewed the wrong version. Sorry.

=> I added a REBASED file to avoid this problem in future.

doc/guide/classify.xml

The description should mention that the IP address can be v4, v6 or v4 mapped v6.

=> added.

src/lib/eval/Makefile.am & src/lib/eval/tests/Makefile.am

I'm unclear as to why there are a number of new libraries being added (dns, crypto etc). Are these actually necessary?

=> yes according the coding guide (they are indirect dependencies).

src/lib/eval/lexer.ll

If I understand the patterns for addr4 and addr6 we are being permissive in what we will consider an IP address and then tossing strings that aren't addresses based on trying to create an IOAddress.

=> yes, it is hard (and a not goal) to get a pattern which accepts all and only IP addresses.

This should be commented. I'm thinking of a line just before the convenience expressions for them.

=> added explanations.

src/lib/eval/tests/context_unittest.cc

There is a test for ::<something and :: there should be a test for <something>:: (the zeros at the end of the address)

=> it is not a random ::<something> but an IPv4-compatible IPv6 address. But as <something>:: is heavily used (as prefixes) I added an unit test.

src/lib/eval/tests/token_unittest.cc

The test is now for "ipaddress" and not "ipaddress6". It's name should probably be updated.

=> fixed.

src/lib/eval/token.cc

In the constructor for TokenIpAddress? we simply ignore any throws from creating the IOAddress. I assume this is because we already validated the string as an address in lexer.ll. It would be useful to add a comment if that is the case.

=> IMHO the comment should be in the design: Token constructors should be not raised exceptions: bad inputs are caught by the scanner and sanity checks should set the value to the empty string. BTW it is what TokenHexString? does.

It will need a Change Log entry. I suggest:
Added support for IP address (v4 and v6) literals in classification rules.

=> I like it (with s/v([46])/IPv\1/g and perhaps s/rules/expressions/)

comment:14 Changed 20 months ago by stephen

  • Owner changed from stephen to sar

I think this ticket should be back with you.

comment:15 Changed 20 months ago by sar

  • Owner changed from sar to fdupont

Your changes to the change log entry look fine.

The rest of the changes look correct, I've run the compile and make check and all seems well.

Tis ready for merging.

comment:16 Changed 20 months ago by sar

I also made a small typo change in classify.xml, please do a pull.

comment:17 Changed 20 months ago by fdupont

  • Resolution set to complete
  • Status changed from reviewing to closed

Pull, merged, closing.

Note: See TracTickets for help on using tickets.