Including RDS in aws representation

[haverma]

Includes:
1)changes for RDS instance
2)changes for allocation of unique Ips from subnet CIDR

---

This change is 

[ratulm]

Looks great.

One question: Possible to add an end-to-end test that shows reachability for the RDS instance? Will test the internal plumbing.

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[arifogel]

Needs some cleanup. I agree with @ratulm that something a bit more end-to-end would may be useful.
I believe there are existing junit tests for AWS networks wrt Instances and VpnGateways you can work from, or possibly extend.

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

---

a discussion (no related file):
Meta-question: Why aren't we using Jackson for all of this?

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 57 at r1 (raw file):

    _multiAz = jObj.getBoolean(JSON_KEY_MULTI_AZ);
    if (jObj.getString(JSON_KEY_DB_INSTANCE_STATUS).toLowerCase().equals("available")) {
      _dbInstanceStatus = Status.AVAILABLE;

What is the value of _dbInstanceStatus if the guard isn't true?

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 100 at r1 (raw file):

    for (int index = 0; index < securityGroupsArray.length(); index++) {
      JSONObject securityGroup = securityGroupsArray.getJSONObject(index);
      if (securityGroup.getString(JSON_KEY_STATUS).equals("active")) {

Should we be case-sensitive here? This is inconsistent with below.

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 109 at r1 (raw file):

    for (int i = 0; i < subnetsArray.length(); i++) {
      JSONObject subnet = subnetsArray.getJSONObject(i);
      if (subnet.getString(JSON_KEY_SUBNET_STATUS).equals("Active")) {

Should we be case-sensitive here? This is inconsistent with above.

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 139 at r1 (raw file):

    _outAcl = new IpAccessList(sgEgressAclName, outboundRules);
    cfgNode.getIpAccessLists().put(sgIngressAclName, _inAcl);
    cfgNode.getIpAccessLists().put(sgEgressAclName, _outAcl);

Why is any of this generation code here? Aren't security groups independent of RDS instances? Why generate these ACLs for everything that uses them?

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 151 at r1 (raw file):

      Subnet subnet = region.getSubnets().get(subnetId);
      String instancesIfaceName = String.format("%s-%s", _dbInstanceIdentifier, subnetId);
      Ip instancesIfaceIp = subnet.getNextIp();

How do you know no one is using this IP?

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 248 at r1 (raw file):

      AwsConfiguration awsConfiguration, Map<String, Configuration> configurationNodes) {

    // updating the list of allocated IPs in the subnet

What subnet? Isn't there more than one?

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 304 at r1 (raw file):

  }

  // updates the Ips which have been allocated already in the subnet

This doesn't provide any more information than the comment in the caller.

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 316 at r1 (raw file):

                      .keySet()
                      .stream()
                      .map(ip -> ip.asLong())

ip -> ip.asLong() -> Ip::asLong

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 318 at r1 (raw file):

                      .map(ip -> ip.asLong())
                      .collect(Collectors.toSet());
              _subnets.get(networkInterface.getSubnetId()).getAllocatedIps().addAll(privateIpsLong);

privateIpsLong can be replaced with its definition inline, and the braces and semicolon can be removed.

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 33 at r1 (raw file):

    // checking the count of RDS initialized
    assertThat(rdsList.size(), equalTo(1));

For better messages, change to:
assertThat(rdsList, hasSize(1));

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 46 at r1 (raw file):

    assertThat(rdsInstance.getSecurityGroups(), equalTo(ImmutableList.of("sg-12345")));
    assertThat(rdsInstance.getMultiAz(), equalTo(false));
    assertThat(rdsInstance.getVpcId(), equalTo("vpc-1"));

Please create feature matchers for all of these properties, and use them instead of checking the values of the features directly.
See e.g. BgpAdvertisementMatchers and BgpAdvertisementMatchersImpl.

Keep the new matchers in the batfish project.

---

projects/batfish/src/test/java/org/batfish/representation/aws/SubnetTest.java, line 42 at r1 (raw file):

    assertThat(subnet.getCidrBlock(), equalTo(Prefix.parse("172.31.0.0/20")));
    assertThat(subnet.getId(), equalTo("subnet-1"));
    assertThat(subnet.getVpcId(), equalTo("vpc-1"));

Please create feature matchers for all of these properties, and use them instead of checking the values of the features directly.
See e.g. BgpAdvertisementMatchers and BgpAdvertisementMatchersImpl.

Keep the new matchers in the batfish project.

---

Comments from Reviewable

[arifogel]

Review status: 7 of 10 files reviewed at latest revision, 14 unresolved discussions.

---

projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 50 at r2 (raw file):

  Ip getNextIp() {
    // skipping (startIp+1) as it is used as the default gateway for instances in this subnet
    for (Long ipAsLong = _cidrBlock.getStartIp().asLong() + 2;

Instead of starting at beginning, start at last generated IP. Reduces from O(n^2) to O(n).

---

Comments from Reviewable

[arifogel]

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 13 unresolved discussions.

---

Comments from Reviewable

[arifogel]

Reviewed 9 of 9 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[arifogel]

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Comments from Reviewable

[haverma]

@ratulm, I have added tests in RDSInstanceTest class to test connection of RDS to subnet node and existence of default routes, also to check if IPs allocated to RDS were unique

---

Review status: 15 of 23 files reviewed at latest revision, 12 unresolved discussions.

---

a discussion (no related file):

Previously, arifogel (Ari Fogel) wrote…

Meta-question: Why aren't we using Jackson for all of this?

I think we continued using Jettison for all the aws json configuration deserialization, opened ticket #875 for this

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 57 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

What is the value of _dbInstanceStatus if the guard isn't true?

it will be Status.UNAVAILABLE for all other values as there were lots of possible states so I tried encompassing all of them with this.
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Overview.DBInstance.Status.html

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 100 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Should we be case-sensitive here? This is inconsistent with below.

done

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 109 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Should we be case-sensitive here? This is inconsistent with above.

done

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 248 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

What subnet? Isn't there more than one?

done

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 304 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This doesn't provide any more information than the comment in the caller.

removed

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 316 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

ip -> ip.asLong() -> Ip::asLong

done

---

projects/batfish/src/main/java/org/batfish/representation/aws/Region.java, line 318 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

privateIpsLong can be replaced with its definition inline, and the braces and semicolon can be removed.

done

---

projects/batfish/src/main/java/org/batfish/representation/aws/Subnet.java, line 50 at r2 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Instead of starting at beginning, start at last generated IP. Reduces from O(n^2) to O(n).

done

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 33 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

For better messages, change to:
assertThat(rdsList, hasSize(1));

done

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 46 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Please create feature matchers for all of these properties, and use them instead of checking the values of the features directly.
See e.g. BgpAdvertisementMatchers and BgpAdvertisementMatchersImpl.

Keep the new matchers in the batfish project.

done

---

projects/batfish/src/test/java/org/batfish/representation/aws/SubnetTest.java, line 42 at r1 (raw file):

Previously, arifogel (Ari Fogel) wrote…

Please create feature matchers for all of these properties, and use them instead of checking the values of the features directly.
See e.g. BgpAdvertisementMatchers and BgpAdvertisementMatchersImpl.

Keep the new matchers in the batfish project.

done

---

Comments from Reviewable

[arifogel]

Looking much better. Still a couple concerns.

---

Reviewed 8 of 8 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 57 at r1 (raw file):

Previously, haverma (Harsh Verma) wrote…

it will be Status.UNAVAILABLE for all other values as there were lots of possible states so I tried encompassing all of them with this.
https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/Overview.DBInstance.Status.html

Looks like it will be null in this code, not Status.UNAVAILABLE. Am I missing something?

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 132 at r5 (raw file):

    // check that  IPs are unique for all the interfaces
    assertThat(ips, hasSize(3));

This test is a little confusing.
Why not collect the IPs of all interfaces into a list. Then convert to set, and make sure they have the same size?
Something like:

List<Ip> ipsAsList = configurations.values();.stream().map(Configuration::getInterfaces).map(Map::values).flatMap(Collection::stream).map(Interface::getAllAddresses).flatMap(Collection::stream).map(NetworkAddress::getIp).collect(ImmutableList.toImmutableList());
Set<Ip> ipsAsSet = ImmutableSet.copyOf(ipsAsList);
assertThat(ipsAsList, hasSize(ipsAsSet.size());

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 139 at r5 (raw file):

    Map<String, Configuration> configurations = loadAwsConfigurations();
    StaticRoute defaultRoute1 = _staticRouteBuilder.setNextHopIp(new Ip("172.31.0.1")).build();
    StaticRoute defaultRoute2 = _staticRouteBuilder.setNextHopIp(new Ip("192.168.2.17")).build();

What a strange nextHopIp. Are you sure that is the intended default gateway (subnet router)? I thought it was supposed to be the second address in the subnet.

---

Comments from Reviewable

[haverma]

Done some changes

---

Review status: 22 of 23 files reviewed at latest revision, 3 unresolved discussions.

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 57 at r1 (raw file):

private Status _dbInstanceStatus = Status.UNAVAILABLE;
I am assigning a default value to it as Status.UNAVAILABLE while declaring it, thought it would be cleaner

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 132 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

This test is a little confusing.
Why not collect the IPs of all interfaces into a list. Then convert to set, and make sure they have the same size?
Something like:

List<Ip> ipsAsList = configurations.values();.stream().map(Configuration::getInterfaces).map(Map::values).flatMap(Collection::stream).map(Interface::getAllAddresses).flatMap(Collection::stream).map(NetworkAddress::getIp).collect(ImmutableList.toImmutableList());
Set<Ip> ipsAsSet = ImmutableSet.copyOf(ipsAsList);
assertThat(ipsAsList, hasSize(ipsAsSet.size());

replaced with suggested snippet

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 139 at r5 (raw file):

Previously, arifogel (Ari Fogel) wrote…

What a strange nextHopIp. Are you sure that is the intended default gateway (subnet router)? I thought it was supposed to be the second address in the subnet.

It is the first Ip in the subnet as is being returned by computeInstanceIfaceIp() function of Subnet.java, that is why Subnet::nextIp was starting from the second Ip in the subnet(now it starts from lastGeneratedIp+1)

---

Comments from Reviewable

[arifogel]

---

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

projects/batfish/src/main/java/org/batfish/representation/aws/RdsInstance.java, line 57 at r1 (raw file):

Previously, haverma (Harsh Verma) wrote…

private Status _dbInstanceStatus = Status.UNAVAILABLE;
I am assigning a default value to it as Status.UNAVAILABLE while declaring it, thought it would be cleaner

OK, my mistake. That's not the pattern in the rest of the project, but it is the pattern in this package .

---

projects/batfish/src/test/java/org/batfish/representation/aws/RdsInstanceTest.java, line 139 at r5 (raw file):

Previously, haverma (Harsh Verma) wrote…

It is the first Ip in the subnet as is being returned by computeInstanceIfaceIp() function of Subnet.java, that is why Subnet::nextIp was starting from the second Ip in the subnet(now it starts from lastGeneratedIp+1)

Misread the json file. Looks good.

---

Comments from Reviewable