Opened 9 years ago

Closed 9 years ago

#979 closed task (complete)

Logic operator ACLs

Reported by: vorner Owned by: vorner
Priority: medium Milestone: Sprint-20110628
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: ACL
Estimated Difficulty: 4.0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

This task would implement the OR and AND (or "ANY-OF" and "ALL-OF", depending on the name we choose) ACL operators (they are together, because they are almost identical and they should share most of their code). This will use #978 to load create sub-expressions. It includes moth the classes for ACLs as well as factory functions for them.

It also depends on #977.

Subtickets

Change History (11)

comment:1 Changed 9 years ago by stephen

  • Milestone changed from Next-Sprint-Proposed to Sprint-20110614

comment:2 Changed 9 years ago by stephen

  • Type changed from defect to task

comment:3 Changed 9 years ago by stephen

  • Estimated Difficulty changed from 0.0 to 4

comment:4 Changed 9 years ago by vorner

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

comment:5 Changed 9 years ago by vorner

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

Hello

This is ready for review. It contains small change to the loader interface (constyfiing few of its methods), but nothing important.

The creator which loads it from JSON is ready to be used, but it is not yet plugged into the default DNS loader. I expect to add the one line when this branch meats with #769.

When I started this, the loader (#trac978) wasn't yet merged. I did get a go for it since, but I didn't merge this with master to make the history cleaner. The first commit on this branch is 85b06e8c212c9733cc77e71d8a72c72161dc34f2 and the whole log/diff/whatever can be extracted with „git log origin/trac978...“.

Thanks

comment:6 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to vorner

logic_check.h

getSubexpressions(): &**i is correct, although shared_ptr does provide the "get()" method to return the pointer to the element. So "i->get()" could also be considered (and may be preferred as being less cryptic).

logic_check_test.cc
Are we allowing nested expressions, e.g.

{ALL: [
    {ANY: [
        {"src-ip": "1.2.3.4"},
        {"src-ip": "5.6.7.8"}
    ]},
    {"dest-ip": "9.10.11.12"}
]}

If so, there should be a unit test for them.

What logic result do we expect an empty expression to come back with? There should be a test for it.

With the sub-expression tests, I think tests should be added to check that both logic operators can return both true and false:

  • AnyOf returns false if all elements are false
  • AllOf returns true if the all elements are true.

comment:8 Changed 9 years ago by stephen

Forgot to mention that tests/Makefile.am needed to have libexceptions and libcc added to the list of libraries to get them to run on my system. I've pushed the updated file.

comment:9 in reply to: ↑ 7 Changed 9 years ago by vorner

  • Owner changed from vorner to stephen

Hello

Replying to stephen:

logic_check.h

getSubexpressions(): &**i is correct, although shared_ptr does provide the "get()" method to return the pointer to the element. So "i->get()" could also be considered (and may be preferred as being less cryptic).

Right, I might have different bar about cryptic code (after teaching perl (therefore learning it (and I want to learn lisp one day too))).

logic_check_test.cc
Are we allowing nested expressions, e.g.
[...]
If so, there should be a unit test for them.

It somehow directly follows from having any expression as subexpression, but I added the test.

What logic result do we expect an empty expression to come back with? There should be a test for it.

If I understand what you mean, the test is already there, LogicCreatorTest::empty.

With the sub-expression tests, I think tests should be added to check that both logic operators can return both true and false:

  • AnyOf returns false if all elements are false
  • AllOf returns true if the all elements are true.

ACK.

comment:10 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to vorner

Right, I might have different bar about cryptic code (after teaching perl (therefore learning it (and I want to learn lisp one day too))).

I've always thought of Perl as a Write Once, Read Never language :-)

All looks good, please merge.

comment:11 in reply to: ↑ 10 Changed 9 years ago by vorner

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

Replying to stephen:

Right, I might have different bar about cryptic code (after teaching perl (therefore learning it (and I want to learn lisp one day too))).

I've always thought of Perl as a Write Once, Read Never language :-)

Yes, well, it's not completely true, there are things that are easier to read when written in Perl than in Java, but reading Perl is definitely harder than writing it. But it still marks whoever uses that language ;-).

All looks good, please merge.

Thanks, done.

Note: See TracTickets for help on using tickets.