Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pan: application-override rule conversion #6651

Merged
merged 36 commits into from Feb 23, 2021
Merged

Pan: application-override rule conversion #6651

merged 36 commits into from Feb 23, 2021

Conversation

sfraint
Copy link
Member

@sfraint sfraint commented Feb 20, 2021

Convert application-override rules and use them in security rule ACLs.

Includes splitting service matching into service and application components since application-override rules can conflict with service matching.


For an application-override rule that looks like this:

set rulebase application-override rules AO_RULE_NAME from z1
set rulebase application-override rules AO_RULE_NAME to z2
set rulebase application-override rules AO_RULE_NAME source ADDR_GRP3
set rulebase application-override rules AO_RULE_NAME destination ADDR_GRP4
set rulebase application-override rules AO_RULE_NAME port 7653
set rulebase application-override rules AO_RULE_NAME protocol tcp
set rulebase application-override rules AO_RULE_NAME application APP_NAME

Trace details can look something like this now:

Matched security rule SR_RULE_NAME
  Matched source address
    Matched address-group ADDR_GRP1
  Matched destination address
    Matched address-group ADDR_GRP2
  Matched service application-default
    Matched application object APP_NAME
      Matched application-override rule AO_RULE_NAME
        Matched source address
          Matched address-group ADDR_GRP3
        Matched destination address
          Matched address-group ADDR_GRP4
        Matched protocol TCP
        Matched port

@batfish-bot
Copy link

This change is Reviewable

Copy link
Member Author

@sfraint sfraint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion (waiting on @sfraint)

a discussion (no related file):
Still need to add a couple more tests: trace detail tests w/ app-override rules, app-override conversion warnings


@codecov
Copy link

codecov bot commented Feb 20, 2021

Codecov Report

Merging #6651 (5a539d9) into master (b8ec4e6) will increase coverage by 0.02%.
The diff coverage is 93.12%.

@@             Coverage Diff              @@
##             master    #6651      +/-   ##
============================================
+ Coverage     73.47%   73.49%   +0.02%     
- Complexity    36506    36544      +38     
============================================
  Files          2919     2920       +1     
  Lines        146988   147178     +190     
  Branches      17729    17755      +26     
============================================
+ Hits         108002   108175     +173     
- Misses        30473    30479       +6     
- Partials       8513     8524      +11     
Impacted Files Coverage Δ Complexity Δ
...epresentation/palo_alto/PaloAltoConfiguration.java 89.32% <92.51%> (+0.76%) 423.00 <34.00> (+31.00)
...resentation/palo_alto/ApplicationOverrideRule.java 100.00% <100.00%> (+2.27%) 25.00 <3.00> (+4.00)
...tation/palo_alto/PaloAltoTraceElementCreators.java 100.00% <100.00%> (+10.52%) 26.00 <1.00> (+2.00)
...atfish/datamodel/FirewallSessionInterfaceInfo.java 91.17% <0.00%> (-4.66%) 14.00% <0.00%> (+1.00%) ⬇️
...ain/java/org/batfish/symbolic/IngressLocation.java 65.78% <0.00%> (-2.64%) 15.00% <0.00%> (-1.00%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 81.81% <0.00%> (-2.28%) 14.00% <0.00%> (-1.00%)
...ava/org/batfish/datamodel/bgp/Layer2VniConfig.java 80.00% <0.00%> (-2.00%) 13.00% <0.00%> (-1.00%)
...a/org/batfish/dataplane/traceroute/FlowTracer.java 92.17% <0.00%> (-1.35%) 104.00% <0.00%> (ø%)
...main/java/org/batfish/datamodel/acl/AclTracer.java 62.89% <0.00%> (-1.26%) 43.00% <0.00%> (-1.00%)
...atfish/bddreachability/SessionInstrumentation.java 87.30% <0.00%> (-1.06%) 27.00% <0.00%> (ø%)
... and 20 more

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 9 of 10 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @corinaminer and @sfraint)

a discussion (no related file):
I didn't see any tests where the same application is mapped to by multiple rules but with different/conflicting service, e.g., my APP can be running on z1>z2 tcp 50 and z3->z2 tcp 70, and it works well.



projects/batfish/src/main/java/org/batfish/representation/palo_alto/ApplicationOverrideRule.java, line 123 at r2 (raw file):

    return _protocol == Protocol.TCP
        ? IpProtocol.TCP
        : _protocol == Protocol.UDP ? IpProtocol.UDP : null;

given enum, probably best as a switch?


projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 1506 at r2 (raw file):

headerspaces

packets? (We don't mean HeaderSpace, right? that class is on its way out (I hope)).

Copy link
Member Author

@sfraint sfraint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 5 of 12 files reviewed, 3 unresolved discussions (waiting on @corinaminer and @dhalperi)

a discussion (no related file):

Previously, sfraint (Spencer Fraint) wrote…

Still need to add a couple more tests: trace detail tests w/ app-override rules, app-override conversion warnings

added


a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

I didn't see any tests where the same application is mapped to by multiple rules but with different/conflicting service, e.g., my APP can be running on z1>z2 tcp 50 and z3->z2 tcp 70, and it works well.

added one in the shadowing test config



projects/batfish/src/main/java/org/batfish/representation/palo_alto/ApplicationOverrideRule.java, line 123 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
    return _protocol == Protocol.TCP
        ? IpProtocol.TCP
        : _protocol == Protocol.UDP ? IpProtocol.UDP : null;

given enum, probably best as a switch?

updated, how's that?


projects/batfish/src/main/java/org/batfish/representation/palo_alto/PaloAltoConfiguration.java, line 1506 at r2 (raw file):

Previously, dhalperi (Dan Halperin) wrote…
headerspaces

packets? (We don't mean HeaderSpace, right? that class is on its way out (I hope)).

right, updated

Copy link
Member

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @corinaminer and @dhalperi)

@sfraint sfraint merged commit e880043 into master Feb 23, 2021
@sfraint sfraint deleted the pan-ao-conversion branch February 23, 2021 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants