Sonic: Deserialize and convert resolve.conf

[ratulm]

After #8204 and #8207

[batfish-bot]

This change is 

[codecov]

Codecov Report

Merging #8208 (3c6c711) into master (e136017) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #8208      +/-   ##
============================================
- Coverage     74.50%   74.49%   -0.01%     
- Complexity    43519    43532      +13     
============================================
  Files          3388     3391       +3     
  Lines        168716   168781      +65     
  Branches      20176    20186      +10     
============================================
+ Hits         125698   125736      +38     
- Misses        33479    33496      +17     
- Partials       9539     9549      +10     

Impacted Files	Coverage Δ
...ndor/sonic/grammar/SonicControlPlaneExtractor.java	93.40% <100.00%> (+0.54%)	⬆️
...tfish/vendor/sonic/representation/ResolveConf.java	100.00% <100.00%> (ø)
...endor/sonic/representation/SonicConfiguration.java	57.14% <100.00%> (+3.53%)	⬆️
.../datamodel/routing_policy/statement/Statement.java	72.72% <0.00%> (-9.10%)	⬇️
...src/main/java/org/batfish/coordinator/PoolMgr.java	54.76% <0.00%> (-5.96%)	⬇️
...fish/bddreachability/BDDLoopDetectionAnalysis.java	83.82% <0.00%> (-2.95%)	⬇️
...ain/java/org/batfish/storage/FileBasedStorage.java	85.47% <0.00%> (-0.86%)	⬇️
...ain/java/org/batfish/coordinator/WorkQueueMgr.java	70.93% <0.00%> (-0.59%)	⬇️
...src/main/java/org/batfish/coordinator/WorkMgr.java	75.50% <0.00%> (-0.47%)	⬇️
.../org/batfish/dataplane/ibdp/BgpRoutingProcess.java	83.67% <0.00%> (-0.09%)	⬇️
... and 6 more

[arifogel]

Reviewed 7 of 17 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dhalperi and @ratulm)

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/grammar/SonicControlPlaneExtractor.java, line 77 at r2 (raw file):

          ResolveConf.deserialize(
              _fileTexts.get(resolveConfFilename),
              _fileResults.get(configDbFilename).getWarnings());

Should this be resolveConfFilename?

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 21 at r2 (raw file):

  private @Nonnull final List<Ip6> _nameservers6;

  public ResolveConf(List<Ip> nameservers, List<Ip6> nameservers6) {

Perhaps make this private and make the immutable copies in deserialize?
I don't see any other usages in this PR.

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 26 at r2 (raw file):

  }

  public static ResolveConf deserialize(String resolveConfText, Warnings warnings) {

nit: @Nonnull return type

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 32 at r2 (raw file):

    String[] lines = resolveConfText.split("\n");
    for (String line : lines) {
      String[] parts = line.split("\\s");

From my read of split javadoc, this won't work with leading whitespace.
Rather than splitting by lines and then by whitespace, why not just iterate over the file with a line regex until no more matches are found?
Something like "(?m)^\\s*nameserver\\s+(^[#\\s]+).*$", then grab group 1.
Note this also handles trailing comments.

[arifogel]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dhalperi and @ratulm)

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 32 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

From my read of split javadoc, this won't work with leading whitespace.
Rather than splitting by lines and then by whitespace, why not just iterate over the file with a line regex until no more matches are found?
Something like "(?m)^\\s*nameserver\\s+(^[#\\s]+).*$", then grab group 1.
Note this also handles trailing comments.

stricter: "(?m)^\\s*nameserver\\s+(^[#\\s]+)\\s*(#.*)?$"

[ratulm]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @arifogel and @dhalperi)

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 32 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

stricter: "(?m)^\\s*nameserver\\s+(^[#\\s]+)\\s*(#.*)?$"

made it robust to extra whitespace while keeping the line-by-line splitting approach.

are those trailing comments legal? i couldn't find a pointer.

[arifogel]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @ratulm)

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 32 at r2 (raw file):

Previously, ratulm wrote…

made it robust to extra whitespace while keeping the line-by-line splitting approach.

are those trailing comments legal? i couldn't find a pointer.

Yes they are valid, just tested. They are generally valid in all linux .conf files

[arifogel]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi and @ratulm)

---

projects/batfish/src/main/java/org/batfish/vendor/sonic/representation/ResolveConf.java, line 32 at r2 (raw file):
Also, on linux (Ubuntu 18.04 at least), only up to 3 nameservers may be used:

Up to MAXNS (currently 3, see <resolv.h>) name servers may be listed, one per keyword.

It's not clear if that's 3 ipv4 and 3 ipv6, or 3 total. My guess is total.

We can probably defer limiting for now.

[arifogel]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dhalperi)