PAN: Parsing and extracting BGP import and export policies #4994
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #4994 +/- ##
============================================
- Coverage 76.7% 76.62% -0.08%
+ Complexity 30197 30024 -173
============================================
Files 2399 2399
Lines 116183 115658 -525
Branches 13720 13662 -58
============================================
- Hits 89115 88620 -495
+ Misses 20338 20324 -14
+ Partials 6730 6714 -16
|
There was a problem hiding this comment.
Reviewed 11 of 17 files at r1.
Reviewable status: 11 of 17 files reviewed, 7 unresolved discussions (waiting on @haverma and @sfraint)
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_bgp.g4, line 217 at r1 (raw file):
RULES name = variable
I'd suggest moving this (and the same keyword + var below) into the bgp_policy_rule
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_bgp.g4, line 217 at r1 (raw file):
bgp_policy_rule
Missing a trailing ??
Same comment applies in most places in PaloAlto_policy_rule - PAN tends to allow you to stop at any point in the hierarchy and this shows up in a lot of places when the device or user defines a structure without fleshing it out fully
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 103 at r1 (raw file):
IP_PREFIX
If you use a rule instead of a token here you can make extraction a little simpler / more uniform.
E.g. add in common.g4
ip_prefix
:
IP_PREFIX
;
Then in extraction you can write a toPrefix(Ip_addressContext ctx) helper
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 110 at r1 (raw file):
Quoted 5 lines of code…
OPEN_BRACKET ( variable )* CLOSE_BRACKET
Do you have to specify a bracketed list here? or can it be a single peer instead? The latter is common elsewhere in PAN grammar
See variable_list in common.g4
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 627 at r1 (raw file):
default: return;
When does this happen? do we need a warning?
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PolicyRule.java, line 16 at r1 (raw file):
private @Nonnull String _name;
nit: final?
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PolicyRule.java, line 43 at r1 (raw file):
public @Nullable @Nullable public
nit: use same ordering for annotations
There was a problem hiding this comment.
Reviewable status: 8 of 18 files reviewed, 1 unresolved discussion (waiting on @sfraint)
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_bgp.g4, line 217 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
RULES name = variableI'd suggest moving this (and the same keyword + var below) into the
bgp_policy_rule
done
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_bgp.g4, line 217 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
bgp_policy_ruleMissing a trailing
??
Same comment applies in most places inPaloAlto_policy_rule- PAN tends to allow you to stop at any point in the hierarchy and this shows up in a lot of places when the device or user defines a structure without fleshing it out fully
done
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 103 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
IP_PREFIXIf you use a rule instead of a token here you can make extraction a little simpler / more uniform.
E.g. add in
common.g4ip_prefix : IP_PREFIX ;Then in extraction you can write a
toPrefix(Ip_addressContext ctx)helper
done
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 110 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
OPEN_BRACKET ( variable )* CLOSE_BRACKETDo you have to specify a bracketed list here? or can it be a single peer instead? The latter is common elsewhere in PAN grammar
See
variable_listincommon.g4
done
projects/batfish/src/main/java/org/batfish/grammar/palo_alto/PaloAltoConfigurationBuilder.java, line 627 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
default: return;When does this happen? do we need a warning?
changed the rule to match only for the expected values, following the pattern used for similar commands in Palo Alto config builder
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PolicyRule.java, line 16 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
private @Nonnull String _name;nit: final?
done
projects/batfish/src/main/java/org/batfish/representation/palo_alto/PolicyRule.java, line 43 at r1 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
public @Nullable @Nullable publicnit: use same ordering for annotations
done
There was a problem hiding this comment.
Reviewed 4 of 17 files at r1, 6 of 6 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @haverma)
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 57 at r2 (raw file):
ALLOW praa_update ;
praa_update seems like it should be optional
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 104 at r2 (raw file):
EXACT yn = yes_or_no
this also seems like it should be optional
There was a problem hiding this comment.
Reviewable status: 17 of 18 files reviewed, 1 unresolved discussion (waiting on @sfraint)
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 57 at r2 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
ALLOW praa_update ;
praa_updateseems like it should be optional
done
projects/batfish/src/main/antlr4/org/batfish/grammar/palo_alto/PaloAlto_policy_rule.g4, line 104 at r2 (raw file):
Previously, sfraint (Spencer Fraint) wrote…
EXACT yn = yes_or_nothis also seems like it should be optional
the UI ensures that exact will be configured for every prefix
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
No description provided.