JunOS: Support As-Path-Group #8613
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @ktaegyum)
projects/batfish/src/main/java/org/batfish/grammar/flatjuniper/ConfigurationBuilder.java line 3007 at r1 (raw file):
public void exitPo_as_path_group(Po_as_path_groupContext ctx) { String name = toString(ctx.name); _configuration.referenceStructure(
as-path-group should not have a self reference, should it? It should be tracked as an unused structure if it's not referred to.
projects/batfish/src/main/java/org/batfish/representation/juniper/AsPathGroup.java line 31 at r1 (raw file):
} public @Nonnull List<String> getAsPathRegexes() {
why is this needed here? This seems like a thing to do in conversion.
projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 3144 at r1 (raw file):
conj.getConjuncts().add(new Disjunction(toBooleanExprs(froms.getFromAsPaths()))); } if (!froms.getFromAsPathGroups().isEmpty()) {
please add a TODO: can you have both as-path and as-path-group together, and how does it work?
projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperStructureUsage.java line 11 at r1 (raw file):
APPLICATION_SET_MEMBER_APPLICATION("application-set member application"), APPLICATION_SET_MEMBER_APPLICATION_SET("application-set member application-set"), AS_PATH_GROUP_SELF_REFERENCE("as-path-group"),
as discussed, drop?
projects/batfish/src/main/java/org/batfish/representation/juniper/PsFromAsPathGroup.java line 39 at r1 (raw file):
asPathGroup .getAsPathRegexes() .forEach(
please just use a regular for loop. This code is so hard to read.
projects/batfish/src/main/java/org/batfish/representation/juniper/PsFromAsPathGroup.java line 44 at r1 (raw file):
org.batfish.representation.juniper.parboiled.AsPathRegex.convertToJavaRegex( regex))); String ORedRegexes = String.join(" | ", javaRegex);
Moving this into the regex is the wrong way to do it. Don't we have an OrExpr?
tests/parsing-tests/networks/unit-tests/configs/juniper_policy_statement line 46 at r1 (raw file):
set policy-options as-path ORIGINATED_IN_65535 ".* 65535" set policy-options as-path UNUSED_ORIGINATED_IN_65535 ".* 65535" set policy-options as-path-group AS_PATH_GROUP as-path ASN714 ".* 714$"
use private ASes for this. Maybe 65535 and 65534?
Code quote:
ASN714 "
|
Previously, dhalperi (Dan Halperin) wrote…
done |
There was a problem hiding this comment.
Reviewable status: 3 of 11 files reviewed, 1 unresolved discussion (waiting on @dhalperi)
projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperConfiguration.java line 3144 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
please add a TODO: can you have both as-path and as-path-group together, and how does it work?
done
projects/batfish/src/main/java/org/batfish/representation/juniper/PsFromAsPathGroup.java line 39 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
please just use a regular for loop. This code is so hard to read.
done
projects/batfish/src/main/java/org/batfish/representation/juniper/PsFromAsPathGroup.java line 44 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
Moving this into the regex is the wrong way to do it. Don't we have an
OrExpr?
done
tests/parsing-tests/networks/unit-tests/configs/juniper_policy_statement line 46 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
use private ASes for this. Maybe 65535 and 65534?
done
projects/batfish/src/main/java/org/batfish/representation/juniper/AsPathGroup.java line 31 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
why is this needed here? This seems like a thing to do in conversion.
removed
projects/batfish/src/main/java/org/batfish/representation/juniper/JuniperStructureUsage.java line 11 at r1 (raw file):
Previously, dhalperi (Dan Halperin) wrote…
as discussed, drop?
done
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ktaegyum)
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8613 +/- ##
==========================================
+ Coverage 71.45% 71.47% +0.01%
==========================================
Files 3274 3275 +1
Lines 168922 168947 +25
Branches 19627 19629 +2
==========================================
+ Hits 120704 120752 +48
+ Misses 38943 38920 -23
Partials 9275 9275
|
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ktaegyum)
a discussion (no related file):
Missing test coverage! I think we need to extend the tests here.
and here
There was a problem hiding this comment.
Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @dhalperi)
a discussion (no related file):
Previously, dhalperi (Dan Halperin) wrote…
Missing test coverage! I think we need to extend the tests here.
and here
test added.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status:
Add support for as-path-group in JunOS.