Validate max total delay

2 files changed, 66 insertions, 26 deletions

diff --git a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
index 7687ad1313f..ff965b5a5df 100644
--- a/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
+++ b/config-model-api/src/main/java/com/yahoo/config/application/api/DeploymentSpec.java
@@ -18,6 +18,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
+import java.util.stream.Collectors;
 
 /**
  * Specifies the environments and regions to which an application should be deployed.
@@ -50,12 +51,23 @@ public class DeploymentSpec {
 
     private DeploymentSpec(Optional<String> globalServiceId, UpgradePolicy upgradePolicy, 
                            List<Step> steps, String xmlForm) {
+        validateTotalDelay(steps);
         this.globalServiceId = globalServiceId;
         this.upgradePolicy = upgradePolicy;
         this.steps = ImmutableList.copyOf(completeSteps(new ArrayList<>(steps)));
         this.xmlForm = xmlForm;
     }
     
+    /** Throw an IllegalArgumentException if the total delay exceeds 24 hours */
+    private static void validateTotalDelay(List<Step> steps) {
+        long totalDelaySeconds = steps.stream().filter(step -> step instanceof Delay)
+                                               .mapToLong(delay -> ((Delay)delay).duration().getSeconds())
+                                               .sum();
+        if (totalDelaySeconds > Duration.ofHours(24).getSeconds())
+            throw new IllegalArgumentException("The total delay specified is " + Duration.ofSeconds(totalDelaySeconds) +
+                                               " but max 24 hours is allowed");
+    }
+    
     /** Adds missing required steps and reorders steps to a permissible order */
     private static List<Step> completeSteps(List<Step> steps) {
         // Add staging if required and missing
@@ -104,7 +116,7 @@ public class DeploymentSpec {
     public UpgradePolicy upgradePolicy() { return upgradePolicy; }
 
     /** Returns the deployment steps of this in the order they will be performed */
-    public List<Step> zones() { return steps; }
+    public List<Step> steps() { return steps; }
     
     /** Returns the XML form of this spec, or null if it was not created by fromXml or is the empty spec */
     public String xmlForm() { return xmlForm; }
@@ -147,9 +159,9 @@ public class DeploymentSpec {
             if (environment == Environment.prod) {
                 for (Element stepTag : XML.getChildren(environmentTag)) {
                     if (stepTag.getTagName().equals("delay"))
-                        steps.add(new Delay(Duration.ofSeconds(intAttribute("hours", stepTag) * 60 * 60 +
-                                                               intAttribute("minutes", stepTag) * 60 +
-                                                               intAttribute("seconds", stepTag))));
+                        steps.add(new Delay(Duration.ofSeconds(longAttribute("hours", stepTag) * 60 * 60 +
+                                                               longAttribute("minutes", stepTag) * 60 +
+                                                               longAttribute("seconds", stepTag))));
                     else // a region: deploy step
                         steps.add(new ZoneDeployment(environment, 
                                                      Optional.of(RegionName.from(XML.getValue(stepTag).trim())), 
@@ -169,7 +181,7 @@ public class DeploymentSpec {
     }
     
     /** Returns the given attribute as an integer, or 0 if it is not present */
-    private static int intAttribute(String attributeName, Element tag) {
+    private static long longAttribute(String attributeName, Element tag) {
         String value = tag.getAttribute(attributeName);
         if (value == null || value.isEmpty()) return 0;
         try {
diff --git a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
index cd34331c069..c6bff6124ba 100644
--- a/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
+++ b/config-model-api/src/test/java/com/yahoo/config/application/api/DeploymentSpecTest.java
@@ -26,8 +26,8 @@ public class DeploymentSpecTest {
         StringReader r = new StringReader(specXml);
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         assertEquals(specXml, spec.xmlForm());
-        assertEquals(1, spec.zones().size());
-        assertTrue(spec.zones().get(0).deploysTo(Environment.test));
+        assertEquals(1, spec.steps().size());
+        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
         assertFalse(spec.includes(Environment.staging, Optional.empty()));
@@ -44,9 +44,9 @@ public class DeploymentSpecTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(2, spec.zones().size());
-        assertTrue(spec.zones().get(0).deploysTo(Environment.test));
-        assertTrue(spec.zones().get(1).deploysTo(Environment.staging));
+        assertEquals(2, spec.steps().size());
+        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
+        assertTrue(spec.steps().get(1).deploysTo(Environment.staging));
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
         assertTrue(spec.includes(Environment.staging, Optional.empty()));
@@ -66,17 +66,17 @@ public class DeploymentSpecTest {
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(4, spec.zones().size());
+        assertEquals(4, spec.steps().size());
 
-        assertTrue(spec.zones().get(0).deploysTo(Environment.test));
+        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
 
-        assertTrue(spec.zones().get(1).deploysTo(Environment.staging));
+        assertTrue(spec.steps().get(1).deploysTo(Environment.staging));
 
-        assertTrue(spec.zones().get(2).deploysTo(Environment.prod, Optional.of(RegionName.from("us-east1"))));
-        assertFalse(((DeploymentSpec.ZoneDeployment)spec.zones().get(2)).active());
+        assertTrue(spec.steps().get(2).deploysTo(Environment.prod, Optional.of(RegionName.from("us-east1"))));
+        assertFalse(((DeploymentSpec.ZoneDeployment)spec.steps().get(2)).active());
 
-        assertTrue(spec.zones().get(3).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1"))));
-        assertTrue(((DeploymentSpec.ZoneDeployment)spec.zones().get(3)).active());
+        assertTrue(spec.steps().get(3).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1"))));
+        assertTrue(((DeploymentSpec.ZoneDeployment)spec.steps().get(3)).active());
 
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
@@ -97,23 +97,27 @@ public class DeploymentSpecTest {
         "   <staging/>" +
         "   <prod>" +
         "      <region active='false'>us-east1</region>" +
+        "      <delay hours='3' minutes='30'/>" + 
         "      <region active='true'>us-west1</region>" +
         "   </prod>" +
         "</deployment>"
         );
 
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
-        assertEquals(4, spec.zones().size());
+        assertEquals(5, spec.steps().size());
 
-        assertTrue(spec.zones().get(0).deploysTo(Environment.test));
+        assertTrue(spec.steps().get(0).deploysTo(Environment.test));
 
-        assertTrue(spec.zones().get(1).deploysTo(Environment.staging));
+        assertTrue(spec.steps().get(1).deploysTo(Environment.staging));
 
-        assertTrue(spec.zones().get(2).deploysTo(Environment.prod, Optional.of(RegionName.from("us-east1"))));
-        assertFalse(((DeploymentSpec.ZoneDeployment)spec.zones().get(2)).active());
+        assertTrue(spec.steps().get(2).deploysTo(Environment.prod, Optional.of(RegionName.from("us-east1"))));
+        assertFalse(((DeploymentSpec.ZoneDeployment)spec.steps().get(2)).active());
 
-        assertTrue(spec.zones().get(3).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1"))));
-        assertTrue(((DeploymentSpec.ZoneDeployment)spec.zones().get(3)).active());
+        assertTrue(spec.steps().get(3) instanceof DeploymentSpec.Delay);
+        assertEquals(3 * 60 * 60 + 30 * 60, ((DeploymentSpec.Delay)spec.steps().get(3)).duration().getSeconds());
+
+        assertTrue(spec.steps().get(4).deploysTo(Environment.prod, Optional.of(RegionName.from("us-west1"))));
+        assertTrue(((DeploymentSpec.ZoneDeployment)spec.steps().get(4)).active());
 
         assertTrue(spec.includes(Environment.test, Optional.empty()));
         assertFalse(spec.includes(Environment.test, Optional.of(RegionName.from("region1"))));
@@ -193,12 +197,36 @@ public class DeploymentSpecTest {
         DeploymentSpec spec = DeploymentSpec.fromXml(r);
         assertEquals("canary", spec.upgradePolicy().toString());
     }
-    
+
+    @Test
+    public void maxDelayExceeded() {
+        try {
+            StringReader r = new StringReader(
+                    "<deployment>" +
+                    "  <upgrade policy='canary'/>" +
+                    "  <prod>" +
+                    "    <region active='true'>us-west-1</region>" +
+                    "    <delay hours='23'/>" +
+                    "    <region active='true'>us-central-1</region>" +
+                    "    <delay minutes='59' seconds='61'/>" +
+                    "    <region active='true'>us-east-3</region>" +
+                    "  </prod>" +
+                    "</deployment>"
+            );
+            DeploymentSpec.fromXml(r);
+            fail("Expected exception due to exceeding the max total delay");
+        }
+        catch (IllegalArgumentException e) {
+            // success
+            assertEquals("The total delay specified is PT24H1S but max 24 hours is allowed", e.getMessage());
+        }
+    }
+
     @Test
     public void testEmpty() {
         assertFalse(DeploymentSpec.empty.globalServiceId().isPresent());
         assertEquals(DeploymentSpec.UpgradePolicy.defaultPolicy, DeploymentSpec.empty.upgradePolicy());
-        assertTrue(DeploymentSpec.empty.zones().isEmpty());
+        assertTrue(DeploymentSpec.empty.steps().isEmpty());
         assertEquals("<deployment version='1.0'/>", DeploymentSpec.empty.xmlForm());
     }