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

CheckPoint: cluster member policy inheritance #7653

Merged
merged 5 commits into from Nov 4, 2021

Conversation

sfraint
Copy link
Member

@sfraint sfraint commented Nov 4, 2021

For cluster members, apply the policy from the cluster.

@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Nov 4, 2021

Codecov Report

Merging #7653 (7bbd4f3) into master (9cd68df) will decrease coverage by 0.00%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #7653      +/-   ##
============================================
- Coverage     73.71%   73.70%   -0.01%     
+ Complexity    41552    41547       -5     
============================================
  Files          3264     3264              
  Lines        163796   163800       +4     
  Branches      19669    19669              
============================================
- Hits         120738   120733       -5     
- Misses        33621    33625       +4     
- Partials       9437     9442       +5     
Impacted Files Coverage Δ
...representation/CheckPointGatewayConfiguration.java 89.43% <88.88%> (+0.10%) ⬆️
...src/main/java/org/batfish/coordinator/PoolMgr.java 54.76% <0.00%> (-4.77%) ⬇️
...fish/bddreachability/BDDLoopDetectionAnalysis.java 80.23% <0.00%> (-2.33%) ⬇️
...bddreachability/BDDReachabilityGraphOptimizer.java 82.82% <0.00%> (-1.02%) ⬇️
...ain/java/org/batfish/coordinator/WorkQueueMgr.java 70.93% <0.00%> (-0.59%) ⬇️
...rg/batfish/dataplane/ibdp/EigrpRoutingProcess.java 89.08% <0.00%> (-0.36%) ⬇️
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.44% <0.00%> (-0.31%) ⬇️
...ain/java/org/batfish/storage/FileBasedStorage.java 86.06% <0.00%> (+0.61%) ⬆️
...ava/org/batfish/datamodel/bgp/Layer2VniConfig.java 82.00% <0.00%> (+2.00%) ⬆️

Copy link
Contributor

@corinaminer corinaminer left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arifogel and @sfraint)


projects/batfish/src/main/java/org/batfish/vendor/check_point_gateway/representation/CheckPointGatewayConfiguration.java, line 641 at r2 (raw file):

        cluster.isPresent()
            ? cluster.get().getPolicy().getAccessPolicyName()
            : gateway.getPolicy().getAccessPolicyName();

did we determine that the cluster member can't have a policy configured? or have we just not seen that?

If the latter, I think we should at least check and warn if a policy is configured on the cluster member

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: all files reviewed, 1 unresolved discussion (waiting on @arifogel and @corinaminer)


projects/batfish/src/main/java/org/batfish/vendor/check_point_gateway/representation/CheckPointGatewayConfiguration.java, line 641 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

did we determine that the cluster member can't have a policy configured? or have we just not seen that?

If the latter, I think we should at least check and warn if a policy is configured on the cluster member

It sounds like the policy should be defined on the cluster object. So, the members should never have policies directly attached.

Copy link
Contributor

@corinaminer corinaminer 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @arifogel)


projects/batfish/src/main/java/org/batfish/vendor/check_point_gateway/representation/CheckPointGatewayConfiguration.java, line 641 at r2 (raw file):

Previously, sfraint (Spencer Fraint) wrote…

It sounds like the policy should be defined on the cluster object. So, the members should never have policies directly attached.

ok, easy enough to change later if we were to out differently

Copy link
Contributor

@corinaminer corinaminer 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @arifogel)


projects/batfish/src/main/java/org/batfish/vendor/check_point_gateway/representation/CheckPointGatewayConfiguration.java, line 641 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

ok, easy enough to change later if we were to out differently

*find out

@sfraint sfraint merged commit 542ebea into master Nov 4, 2021
@sfraint sfraint deleted the checkpoint-cluster-member-policy branch November 4, 2021 19:54
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