Merge pull request #323 from ibi-group/otp-runner-quoting

Properly escape otp-runner manifest file

src/main/java/com/conveyal/datatools/manager/jobs/DeployJob.java

@@ -4,6 +4,7 @@
 import com.amazonaws.auth.AWSStaticCredentialsProvider;
 import com.amazonaws.event.ProgressListener;
 import com.amazonaws.services.ec2.AmazonEC2;
+import com.amazonaws.services.ec2.model.AmazonEC2Exception;
 import com.amazonaws.services.ec2.model.CreateImageRequest;
 import com.amazonaws.services.ec2.model.CreateImageResult;
 import com.amazonaws.services.ec2.model.CreateTagsRequest;
@@ -717,7 +718,15 @@ private List<Instance> startEC2Instances(int count, boolean graphAlreadyBuilt) {
                 // This will have the instance terminate when it is shut down.
                 .withInstanceInitiatedShutdownBehavior("terminate")
                 .withUserData(Base64.encodeBase64String(userData.getBytes()));
-        final List<Instance> instances = ec2.runInstances(runInstancesRequest).getReservation().getInstances();
+        List<Instance> instances;
+        try {
+            // attempt to start the instances. Sometimes, AWS does not have enough availability of the desired instance
+            // type and can throw an error at this point.
+            instances = ec2.runInstances(runInstancesRequest).getReservation().getInstances();
+        } catch (AmazonEC2Exception e) {
+            status.fail(String.format("DeployJob failed due to a problem with AWS: %s", e.getMessage()), e);
+            return Collections.EMPTY_LIST;
+        }
 
         List<String> instanceIds = getIds(instances);
         Set<String> instanceIpAddresses = new HashSet<>();
@@ -794,7 +803,7 @@ private String getRouterId() {
      * Construct the user data script (as string) that should be provided to the AMI and executed upon EC2 instance
      * startup.
      */
-    private String constructUserData(boolean graphAlreadyBuilt) {
+    public String constructUserData(boolean graphAlreadyBuilt) {
         if (deployment.r5) {
             status.fail("Deployments with r5 are not supported at this time");
             return null;
@@ -822,7 +831,7 @@ private String constructUserData(boolean graphAlreadyBuilt) {
         manifest.uploadOtpRunnerLogs = true;
         if (!graphAlreadyBuilt) {
             // settings when graph building needs to happen
-            manifest.buildConfigJSON = deployment.generateBuildConfigAsString();
+            manifest.buildConfigJSON = createEchoSafeJSON(deployment.generateBuildConfigAsString());
             manifest.buildGraph = true;
             try {
                 manifest.gtfsAndOsmUrls = deployment.generateGtfsAndOsmUrls();
@@ -838,14 +847,14 @@ private String constructUserData(boolean graphAlreadyBuilt) {
                 manifest.runServer = false;
             } else {
                 // This instance will both run a graph and start the OTP server
-                manifest.routerConfigJSON = deployment.generateRouterConfigAsString();
+                manifest.routerConfigJSON = createEchoSafeJSON(deployment.generateRouterConfigAsString());
                 manifest.runServer = true;
                 manifest.uploadServerStartupLogs = true;
             }
         } else {
             // This instance will only start the OTP server with a prebuilt graph
             manifest.buildGraph = false;
-            manifest.routerConfigJSON = deployment.generateRouterConfigAsString();
+            manifest.routerConfigJSON = createEchoSafeJSON(deployment.generateRouterConfigAsString());
             manifest.runServer = true;
             manifest.uploadServerStartupLogs = true;
         }
@@ -894,6 +903,13 @@ private String constructUserData(boolean graphAlreadyBuilt) {
         return String.join("\n", lines);
     }
 
+    /**
+     * Provide proper escaping to stringified JSON so that it can be properly included in the otp-runner manifest.json
+     */
+    private String createEchoSafeJSON(String json) {
+        return json.replace("\"", "\\\"").replace("\n", "\\n");
+    }
+
     private String getJarName() {
         String jarName = deployment.r5 ? deployment.r5Version : deployment.otpVersion;
         if (jarName == null) {

src/main/java/com/conveyal/datatools/manager/models/Deployment.java

@@ -282,7 +282,7 @@ public Deployment(Project project) {
     }
 
     /**
-     * Create an empty deployment, for use with dump/restore.
+     * Create an empty deployment, for use with dump/restore and testing.
      */
     public Deployment() {
         // do nothing.
@@ -550,11 +550,13 @@ public boolean delete() {
      */
     public List<String> generateGtfsAndOsmUrls() throws MalformedURLException {
         Set<String> urls = new HashSet<>();
-        // add OSM data
-        urls.add(getVexUrl(retrieveProjectBounds()).toString());
-        // add GTFS files
-        for (String feedVersionId : feedVersionIds) {
-            urls.add(feedStore.getS3FeedPath(feedVersionId));
+        if (feedVersionIds.size() > 0) {
+            // add OSM data
+            urls.add(getVexUrl(retrieveProjectBounds()).toString());
+            // add GTFS files
+            for (String feedVersionId : feedVersionIds) {
+                urls.add(feedStore.getS3FeedPath(feedVersionId));
+            }
         }
         return new ArrayList<>(urls);
     }

src/test/java/com/conveyal/datatools/manager/jobs/DeployJobTest.java

@@ -18,21 +18,25 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.Date;
 import java.util.List;
 
+import static com.conveyal.datatools.TestUtils.getBooleanEnvVar;
 import static com.conveyal.datatools.manager.controllers.api.ServerController.getIds;
 import static com.conveyal.datatools.manager.controllers.api.ServerController.terminateInstances;
+import static com.zenika.snapshotmatcher.SnapshotMatcher.matchesSnapshot;
+import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assume.assumeTrue;
 
 /**
- * This contains a few helpful tests for quickly spinning up a deployment using any bundle of GTFS + OSM or pre-built
- * graph that already exists on S3.
- *
- * Note: these tests require credentials on the IBI AWS account, which is why the class is tagged with Ignore (so that it is
- * not run with the rest of the test suite)
+ * This test suite contains various unit and integration tests to make sure the a DeployJob works properly. The unit
+ * tests are always ran as part of the unit test suite and don't require AWS access. However, some tests will use AWS
+ * services to run an integrations test of deploying to AWS infrastructure. For the tests that require credentials on
+ * the IBI AWS account, these requires changing server.yml#modules.deployment.ec2.enabled to true and also that the
+ * RUN_AWS_DEPLOY_JOB_TESTS environment variable is set to "true".
  */
-@Ignore
 public class DeployJobTest extends UnitTest {
     private static final Logger LOG = LoggerFactory.getLogger(DeployJobTest.class);
     private static Project project;
@@ -64,19 +68,58 @@ public static void setUp() throws IOException {
         server.ec2Info.iamInstanceProfileArn = "YOUR_VALUE";
         Persistence.servers.create(server);
         deployment = new Deployment();
+        // the feedVersionIds variable does not get set during tests resulting in a null pointer exception. Set the
+        // feedVersionIds to an empty list to avoid this. It is enough to run the tests of the user data generation with
+        // an empty list of feed versions, so it is fine that it is empty.
+        deployment.feedVersionIds = new ArrayList<>();
         deployment.projectId = project.id;
         deployment.name = "Test deployment";
         deployment.otpVersion = "otp-latest-trimet-dev";
+        // add in custom build and router config with problematic characters to make sure they are properly escaped
+        deployment.customRouterConfig = "{ \"hi\": \"th\ne're\" }";
+        deployment.customBuildConfig = "{ \"hello\": \"th\ne're\" }";
         Persistence.deployments.create(deployment);
     }
 
+    /**
+     * Tests that the user data for a graph build + run server instance can be generated properly
+     */
+    @Test
+    public void canMakeGraphBuildUserDataScript () {
+        DeployJob deployJob = new DeployJob(
+            deployment,
+            Auth0UserProfile.createTestAdminUser(),
+            server,
+            "test-deploy",
+            DeployJob.DeployType.REPLACE
+        );
+        assertThat(deployJob.constructUserData(false), matchesSnapshot());
+    }
+
+    /**
+     * Tests that the user data for a run server only instance can be generated properly
+     */
+    @Test
+    public void canMakeServerOnlyUserDataScript () {
+        DeployJob deployJob = new DeployJob(
+            deployment,
+            Auth0UserProfile.createTestAdminUser(),
+            server,
+            "test-deploy",
+            DeployJob.DeployType.REPLACE
+        );
+        assertThat(deployJob.constructUserData(true), matchesSnapshot());
+    }
+
     /**
      * Tests that Data Tools can run an ELB deployment.
      *
-     * Note: this requires changing server.yml#modules.deployment.ec2.enabled to true
+     * Note: this requires changing server.yml#modules.deployment.ec2.enabled to true and also that the
+     * RUN_AWS_DEPLOY_JOB_TESTS environment variable is set to "true"
      */
     @Test
     public void canDeployFromPreloadedBundle () {
+        assumeTrue(getBooleanEnvVar("RUN_AWS_DEPLOY_JOB_TESTS"));
         DeployJob deployJob = new DeployJob(deployment, Auth0UserProfile.createTestAdminUser(), server, "test-deploy", DeployJob.DeployType.USE_PRELOADED_BUNDLE);
         deployJob.run();
         // FIXME: Deployment will succeed even if one of the clone servers does not start up properly.
@@ -85,17 +128,28 @@ public void canDeployFromPreloadedBundle () {
 
     /**
      * Tests that Data Tools can run an ELB deployment from a pre-built graph.
+     *
+     * Note: this requires changing server.yml#modules.deployment.ec2.enabled to true and also that the
+     * RUN_AWS_DEPLOY_JOB_TESTS environment variable is set to "true"
      */
     @Test
     public void canDeployFromPrebuiltGraph () {
+        assumeTrue(getBooleanEnvVar("RUN_AWS_DEPLOY_JOB_TESTS"));
         DeployJob deployJob = new DeployJob(deployment, Auth0UserProfile.createTestAdminUser(), server, "deploy-test", DeployJob.DeployType.USE_PREBUILT_GRAPH);
         deployJob.run();
         // FIXME: Deployment will succeed even if one of the clone servers does not start up properly.
         assertFalse("Deployment did not fail.", deployJob.status.error);
     }
 
+    /**
+     * Terminates any instances that were created during the tests.
+     *
+     * Note: this requires changing server.yml#modules.deployment.ec2.enabled to true and also that the
+     * RUN_AWS_DEPLOY_JOB_TESTS environment variable is set to "true"
+     */
     @AfterClass
     public static void cleanUp() {
+        assumeTrue(getBooleanEnvVar("RUN_AWS_DEPLOY_JOB_TESTS"));
         List<Instance> instances = server.retrieveEC2Instances();
         List<String> ids = getIds(instances);
         terminateInstances(

src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/DeployJobTest/canMakeGraphBuildUserDataScript-0.json

@@ -0,0 +1 @@
+"#!/bin/bash\nexport PATH=\"$PATH:/home/ubuntu/.yarn/bin\"\nexport PATH=\"$PATH:/home/ubuntu/.nvm/versions/node/v12.16.3/bin\"\nrm /usr/share/nginx/client/status.json || echo '' > /dev/null\nrm /var/otp/otp-runner-manifest.json || echo '' > /dev/null\nrm /opt/otp/otp-latest-trimet-dev || echo '' > /dev/null\nrm /var/log/otp-runner.log || echo '' > /dev/null\nrm /var/log/otp-build.log || echo '' > /dev/null\nrm /var/log/otp-server.log || echo '' > /dev/null\necho $'{\"buildConfigJSON\":\"{ \\\\\\\"hello\\\\\\\": \\\\\\\"th\\\\ne\\'re\\\\\\\" }\",\"buildGraph\":true,\"graphObjUrl\":\"s3://datatools-dev/test-deploy/Graph.obj\",\"graphsFolder\":\"/var/otp/graphs\",\"jarFile\":\"/opt/otp/otp-latest-trimet-dev\",\"jarUrl\":\"https://opentripplanner-builds.s3.amazonaws.com/otp-latest-trimet-dev.jar\",\"otpRunnerLogFile\":\"/var/log/otp-runner.log\",\"prefixLogUploadsWithInstanceId\":true,\"routerConfigJSON\":\"{ \\\\\\\"hi\\\\\\\": \\\\\\\"th\\\\ne\\'re\\\\\\\" }\",\"runServer\":true,\"s3UploadPath\":\"s3://datatools-dev/test-deploy\",\"serverStartupTimeoutSeconds\":3300,\"statusFileLocation\":\"/usr/share/nginx/client/status.json\",\"uploadGraphBuildLogs\":true,\"uploadGraphBuildReport\":true,\"uploadGraph\":true,\"uploadOtpRunnerLogs\":true,\"uploadServerStartupLogs\":true}' > /var/otp/otp-runner-manifest.json\nyarn global add https://github.com/ibi-group/otp-runner.git\notp-runner /var/otp/otp-runner-manifest.json"

src/test/resources/snapshots/com/conveyal/datatools/manager/jobs/DeployJobTest/canMakeServerOnlyUserDataScript-0.json

@@ -0,0 +1 @@
+"#!/bin/bash\nexport PATH=\"$PATH:/home/ubuntu/.yarn/bin\"\nexport PATH=\"$PATH:/home/ubuntu/.nvm/versions/node/v12.16.3/bin\"\nrm /usr/share/nginx/client/status.json || echo '' > /dev/null\nrm /var/otp/otp-runner-manifest.json || echo '' > /dev/null\nrm /opt/otp/otp-latest-trimet-dev || echo '' > /dev/null\nrm /var/log/otp-runner.log || echo '' > /dev/null\nrm /var/log/otp-build.log || echo '' > /dev/null\nrm /var/log/otp-server.log || echo '' > /dev/null\necho $'{\"buildGraph\":false,\"graphObjUrl\":\"s3://datatools-dev/test-deploy/Graph.obj\",\"graphsFolder\":\"/var/otp/graphs\",\"jarFile\":\"/opt/otp/otp-latest-trimet-dev\",\"jarUrl\":\"https://opentripplanner-builds.s3.amazonaws.com/otp-latest-trimet-dev.jar\",\"otpRunnerLogFile\":\"/var/log/otp-runner.log\",\"prefixLogUploadsWithInstanceId\":true,\"routerConfigJSON\":\"{ \\\\\\\"hi\\\\\\\": \\\\\\\"th\\\\ne\\'re\\\\\\\" }\",\"runServer\":true,\"s3UploadPath\":\"s3://datatools-dev/test-deploy\",\"serverStartupTimeoutSeconds\":3300,\"statusFileLocation\":\"/usr/share/nginx/client/status.json\",\"uploadGraphBuildLogs\":false,\"uploadGraphBuildReport\":false,\"uploadGraph\":false,\"uploadOtpRunnerLogs\":true,\"uploadServerStartupLogs\":true}' > /var/otp/otp-runner-manifest.json\nyarn global add https://github.com/ibi-group/otp-runner.git\notp-runner /var/otp/otp-runner-manifest.json"