chore(exceptions): Fix up what exceptions are thrown (spinnaker#3835)

* chore(exceptions): Fix up what exceptions are thrown In an effor to clean up logs to improve signal to noise ratio of bug vs non-essential/expected failures: * clean up what exceptions are thrown from a bunch of places * anything deriving from UserException won't get logged as a failure * for now, change capacity short circuit to warn instead of error because it is mostly noise. I will work on making it provide accurate signal separately * fixup! chore(exceptions): Fix up what exceptions are thrown
fkyc · Jul 26, 2020 · c47e530 · c47e530
1 parent 14fb0f4
commit c47e530
Show file tree

Hide file tree

Showing 17 changed files with 116 additions and 35 deletions.
diff --git a/.../PreconfiguredJobNotFoundException.groovy → ...on/PreconfiguredJobNotFoundException.java b/.../PreconfiguredJobNotFoundException.groovy → ...on/PreconfiguredJobNotFoundException.java
@@ -15,10 +15,12 @@
  * limitations under the License.
  */
 
-package com.netflix.spinnaker.orca.clouddriver.exception
+package com.netflix.spinnaker.orca.clouddriver.exception;
 
-class PreconfiguredJobNotFoundException extends RuntimeException {
-  PreconfiguredJobNotFoundException(String jobKey) {
-    super("Could not find a stage named '$jobKey'")
+import com.netflix.spinnaker.kork.exceptions.UserException;
+
+public class PreconfiguredJobNotFoundException extends UserException {
+  public PreconfiguredJobNotFoundException(String jobKey) {
+    super("Could not find a stage named '" + jobKey + "'");
   }
 }
diff --git a/...y/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/ClusterSizePreconditionTask.groovy b/...y/com/netflix/spinnaker/orca/clouddriver/tasks/cluster/ClusterSizePreconditionTask.groovy
@@ -18,11 +18,13 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.cluster
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.frigga.Names
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException
 import com.netflix.spinnaker.moniker.Moniker
 import com.netflix.spinnaker.orca.api.pipeline.RetryableTask
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.clouddriver.OortService
+import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException
 import com.netflix.spinnaker.orca.clouddriver.tasks.AbstractCloudProviderAwareTask
 import com.netflix.spinnaker.orca.pipeline.tasks.PreconditionTask
 import groovy.transform.Canonical
@@ -82,7 +84,7 @@ class ClusterSizePreconditionTask extends AbstractCloudProviderAwareTask impleme
         errors << 'Missing regions'
       }
       if (errors) {
-        throw new IllegalStateException("Invalid configuration " + errors.join(','))
+        throw new ConfigurationException("Invalid configuration " + errors.join(', '))
       }
     }
   }
@@ -131,7 +133,7 @@ class ClusterSizePreconditionTask extends AbstractCloudProviderAwareTask impleme
     }
 
     if (failures) {
-      throw new IllegalStateException("Precondition check failed: ${failures.join(', ')}")
+      throw new PreconditionFailureException("Precondition check failed: ${failures.join(', ')}")
     }
 
     return TaskResult.SUCCEEDED

diff --git a/...roovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/WaitForUpInstancesTask.groovy b/...roovy/com/netflix/spinnaker/orca/clouddriver/tasks/instance/WaitForUpInstancesTask.groovy
@@ -243,7 +243,7 @@ class WaitForUpInstancesTask extends AbstractWaitingForInstancesTask {
       if (System.currentTimeMillis() - TimeUnit.MINUTES.toMillis(10) > Long.valueOf(taskStartTime.get())) {
         // expectation is reconciliation has happened within 10 minutes and that the
         // current server group capacity should be preferred
-        log.error(
+        log.warn(
           "Short circuiting initial target capacity determination after 10 minutes (serverGroup: {}, executionId: {})",
           "${cloudProvider}:${serverGroup.region}:${serverGroup.name}",
           stage.execution.id

diff --git a/...m/netflix/spinnaker/orca/clouddriver/tasks/cluster/ClusterSizePreconditionTaskSpec.groovy b/...m/netflix/spinnaker/orca/clouddriver/tasks/cluster/ClusterSizePreconditionTaskSpec.groovy
@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.orca.clouddriver.tasks.cluster
 
+import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException
+
 import java.util.concurrent.atomic.AtomicInteger
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
@@ -109,7 +111,7 @@ class ClusterSizePreconditionTaskSpec extends Specification {
     then:
     1 * oortService.getCluster('foo', 'test', 'foo', 'aws') >> response
 
-    thrown(IllegalStateException)
+    thrown(PreconditionFailureException)
 
     where:
     credentials = 'test'

diff --git a/.../main/java/com/netflix/spinnaker/orca/exceptions/PipelineTemplateValidationException.java b/.../main/java/com/netflix/spinnaker/orca/exceptions/PipelineTemplateValidationException.java
@@ -0,0 +1,40 @@
+/*
+ * Copyright 2020 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.orca.exceptions;
+
+import com.netflix.spinnaker.kork.exceptions.HasAdditionalAttributes;
+import com.netflix.spinnaker.kork.exceptions.UserException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Map;
+
+public final class PipelineTemplateValidationException extends UserException
+    implements HasAdditionalAttributes {
+  private final Collection errors;
+
+  public PipelineTemplateValidationException(String message, Collection errors) {
+    super(message);
+    this.errors = errors;
+  }
+
+  @Override
+  public Map<String, Object> getAdditionalAttributes() {
+    return errors != null && !errors.isEmpty()
+        ? Collections.singletonMap("errors", errors)
+        : Collections.emptyMap();
+  }
+}
diff --git a/...ore/src/main/java/com/netflix/spinnaker/orca/exceptions/PreconditionFailureException.java b/...ore/src/main/java/com/netflix/spinnaker/orca/exceptions/PreconditionFailureException.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright 2020 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.orca.exceptions;
+
+import com.netflix.spinnaker.kork.exceptions.UserException;
+
+public class PreconditionFailureException extends UserException {
+  public PreconditionFailureException(String message) {
+    super(message);
+  }
+}
diff --git a/.../src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/StageStatusPreconditionTask.java b/.../src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/StageStatusPreconditionTask.java
@@ -16,9 +16,11 @@
 
 package com.netflix.spinnaker.orca.pipeline.tasks;
 
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException;
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
+import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException;
 import javax.annotation.Nonnull;
 import lombok.Getter;
 import org.springframework.stereotype.Component;
@@ -38,12 +40,12 @@ public String getPreconditionType() {
     String stageName = context.getStageName();
     String assertedStatus = context.getStageStatus();
     if (stageName == null) {
-      throw new IllegalArgumentException(
+      throw new ConfigurationException(
           String.format(
               "Stage name is required for preconditions of type %s.", getPreconditionType()));
     }
     if (assertedStatus == null) {
-      throw new IllegalArgumentException(
+      throw new ConfigurationException(
           String.format(
               "Stage status is required for preconditions of type %s.", getPreconditionType()));
     }
@@ -53,13 +55,13 @@ public String getPreconditionType() {
             .findFirst()
             .orElseThrow(
                 () ->
-                    new IllegalArgumentException(
+                    new ConfigurationException(
                         String.format(
                             "Failed to find stage %s in execution. Please specify a valid stage name",
                             stageName)));
     String actualStatus = foundStage.getStatus().toString();
     if (!actualStatus.equals(assertedStatus)) {
-      throw new RuntimeException(
+      throw new PreconditionFailureException(
           String.format(
               "The status of stage %s was asserted to be %s, but was actually %s",
               stageName, assertedStatus, actualStatus));

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/V2Util.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipelinetemplate/V2Util.java
@@ -17,8 +17,8 @@
 package com.netflix.spinnaker.orca.pipelinetemplate;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.netflix.spinnaker.kork.web.exceptions.ValidationException;
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
+import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException;
 import com.netflix.spinnaker.orca.extensionpoint.pipeline.ExecutionPreprocessor;
 import com.netflix.spinnaker.orca.pipeline.expressions.PipelineExpressionEvaluator;
 import com.netflix.spinnaker.orca.pipeline.util.ContextParameterProcessor;
@@ -50,7 +50,7 @@ public static Map<String, Object> planPipeline(
 
     List<Map<String, Object>> pipelineErrors = (List<Map<String, Object>>) pipeline.get("errors");
     if (pipelineErrors != null && !pipelineErrors.isEmpty()) {
-      throw new ValidationException("Pipeline template is invalid", pipelineErrors);
+      throw new PipelineTemplateValidationException("Pipeline template is invalid", pipelineErrors);
     }
 
     Map<String, Object> augmentedContext = new HashMap<>();
@@ -72,7 +72,7 @@ public static Map<String, Object> planPipeline(
               .collect(Collectors.toList());
 
       if (failedTemplateVars.size() > 0) {
-        throw new ValidationException(
+        throw new PipelineTemplateValidationException(
             "Missing template variable values for the following variables: %s", failedTemplateVars);
       }
     }

diff --git a/...t/groovy/com/netflix/spinnaker/orca/pipeline/tasks/StageStatusPreconditionTaskSpec.groovy b/...t/groovy/com/netflix/spinnaker/orca/pipeline/tasks/StageStatusPreconditionTaskSpec.groovy
@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.orca.pipeline.tasks
 
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException
+import com.netflix.spinnaker.orca.exceptions.PreconditionFailureException
 import spock.lang.Specification
 import spock.lang.Unroll
 import static com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.SUCCEEDED
@@ -79,7 +81,7 @@ class StageStatusPreconditionTaskSpec extends Specification {
     task.execute(stage)
 
     then:
-    thrown(IllegalArgumentException)
+    thrown(ConfigurationException)
 
     where:
     stageName | stageStatus
@@ -113,7 +115,7 @@ class StageStatusPreconditionTaskSpec extends Specification {
     task.execute(stage)
 
     then:
-    thrown(RuntimeException)
+    thrown(PreconditionFailureException)
 
     where:
     stageName | stageStatus

diff --git a/...ront50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy b/...ront50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy
@@ -19,10 +19,10 @@ package com.netflix.spinnaker.orca.front50
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spectator.api.Id
 import com.netflix.spectator.api.Registry
-import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
-import com.netflix.spinnaker.kork.web.exceptions.ValidationException
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
 import com.netflix.spinnaker.orca.api.pipeline.models.Trigger
+import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException
 import com.netflix.spinnaker.orca.extensionpoint.pipeline.ExecutionPreprocessor
 import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher
 import com.netflix.spinnaker.orca.pipeline.util.ArtifactUtils
@@ -74,7 +74,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
     def json = objectMapper.writeValueAsString(pipelineConfig)
 
     if (pipelineConfig.disabled) {
-      throw new InvalidRequestException("Pipeline '${pipelineConfig.name}' is disabled and cannot be triggered")
+      throw new ConfigurationException("Pipeline '${pipelineConfig.name}' is disabled and cannot be triggered")
     }
 
     log.info('triggering dependent pipeline {}:{}', pipelineConfig.id, json)
@@ -134,7 +134,7 @@ class DependentPipelineStarter implements ApplicationContextAware {
     }
 
     if (pipelineConfig.errors != null) {
-      throw new ValidationException("Pipeline template is invalid", pipelineConfig.errors as List<Map<String, Object>>)
+      throw new PipelineTemplateValidationException("Pipeline template is invalid", pipelineConfig.errors as List<Map<String, Object>>)
     }
 
     // Process the raw trigger to resolve any expressions before converting it to a Trigger object, which will not be

diff --git a/...front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/StartPipelineTask.groovy b/...front50/src/main/groovy/com/netflix/spinnaker/orca/front50/tasks/StartPipelineTask.groovy
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.orca.front50.tasks
 
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.Task
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
@@ -70,11 +71,11 @@ class StartPipelineTask implements Task {
     Map<String, Object> pipelineConfig = pipelines.find { it.id == pipelineId }
 
     if (!pipelineConfig) {
-      throw new IllegalArgumentException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} cannot be located (${pipelineId})")
+      throw new ConfigurationException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} cannot be located (${pipelineId})")
     }
 
     if (pipelineConfig.getOrDefault("disabled", false)) {
-      throw new IllegalArgumentException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} is disabled")
+      throw new ConfigurationException("The referenced ${isStrategy ? 'custom strategy' : 'pipeline'} is disabled")
     }
 
     if (V2Util.isV2Pipeline(pipelineConfig)) {

diff --git a/...lix/spinnaker/orca/pipelinetemplate/exceptions/IllegalTemplateConfigurationException.java b/...lix/spinnaker/orca/pipelinetemplate/exceptions/IllegalTemplateConfigurationException.java
@@ -15,10 +15,11 @@
  */
 package com.netflix.spinnaker.orca.pipelinetemplate.exceptions;
 
+import com.netflix.spinnaker.kork.exceptions.UserException;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
 
-public class IllegalTemplateConfigurationException extends IllegalStateException
+public class IllegalTemplateConfigurationException extends UserException
     implements PipelineTemplateException {
 
   private Error error;

diff --git a/.../java/com/netflix/spinnaker/orca/pipelinetemplate/exceptions/TemplateRenderException.java b/.../java/com/netflix/spinnaker/orca/pipelinetemplate/exceptions/TemplateRenderException.java
@@ -15,10 +15,12 @@
  */
 package com.netflix.spinnaker.orca.pipelinetemplate.exceptions;
 
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors;
 import com.netflix.spinnaker.orca.pipelinetemplate.validator.Errors.Error;
 
-public class TemplateRenderException extends RuntimeException implements PipelineTemplateException {
+public class TemplateRenderException extends ConfigurationException
+    implements PipelineTemplateException {
 
   public static TemplateRenderException fromError(Error error) {
     return new TemplateRenderException(error.getMessage(), null, error);

diff --git a/...r/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransformSpec.groovy b/...r/orca/pipelinetemplate/v1schema/graph/transform/ConfigStageInjectionTransformSpec.groovy
@@ -88,7 +88,7 @@ class ConfigStageInjectionTransformSpec extends Specification {
     ConfigStageInjectionTransform.createGraph(stages)
 
     then:
-    thrown(IllegalStateException)
+    thrown(IllegalTemplateConfigurationException)
   }
 
   def 'should inject stage into dag'() {
@@ -169,7 +169,7 @@ class ConfigStageInjectionTransformSpec extends Specification {
     )).visitPipelineTemplate(template)
 
     then:
-    thrown(IllegalStateException)
+    thrown(IllegalTemplateConfigurationException)
 
     when: 'injecting stage after another stage in dag'
     template = templateBuilder()

diff --git a/...orca/pipelinetemplate/v2schema/graph/transform/V2ConfigStageInjectionTransformSpec.groovy b/...orca/pipelinetemplate/v2schema/graph/transform/V2ConfigStageInjectionTransformSpec.groovy
@@ -91,7 +91,7 @@ class V2ConfigStageInjectionTransformSpec extends Specification {
     V2ConfigStageInjectionTransform.createGraph(stages)
 
     then:
-    thrown(IllegalStateException)
+    thrown(IllegalTemplateConfigurationException)
   }
 
   def 'should inject stage into dag'() {
@@ -174,7 +174,7 @@ class V2ConfigStageInjectionTransformSpec extends Specification {
     )).visitPipelineTemplate(template)
 
     then:
-    thrown(IllegalStateException)
+    thrown(IllegalTemplateConfigurationException)
 
     when: 'injecting stage after another stage in dag'
     template = templateBuilder()

diff --git a/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy b/orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy
@@ -21,12 +21,14 @@ import com.netflix.spinnaker.fiat.model.UserPermission
 import com.netflix.spinnaker.fiat.model.resources.Role
 import com.netflix.spinnaker.fiat.shared.FiatService
 import com.netflix.spinnaker.fiat.shared.FiatStatus
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException
 import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException
 import com.netflix.spinnaker.kork.web.exceptions.ValidationException
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
 import com.netflix.spinnaker.orca.clouddriver.service.JobService
 import com.netflix.spinnaker.orca.exceptions.OperationFailedException
+import com.netflix.spinnaker.orca.exceptions.PipelineTemplateValidationException
 import com.netflix.spinnaker.orca.extensionpoint.pipeline.ExecutionPreprocessor
 import com.netflix.spinnaker.orca.front50.Front50Service
 import com.netflix.spinnaker.orca.front50.PipelineModelMutator
@@ -240,7 +242,7 @@ class OperationsController {
     }
 
     if (pipeline.disabled) {
-      throw new InvalidRequestException("Pipeline is disabled and cannot be started.")
+      throw new ConfigurationException("Pipeline is disabled and cannot be started.")
     }
 
     def linear = pipeline.stages.every { it.refId == null }
@@ -249,7 +251,7 @@ class OperationsController {
     }
 
     if (pipeline.errors != null) {
-      throw new ValidationException("Pipeline template is invalid", pipeline.errors as List<Map<String, Object>>)
+      throw new PipelineTemplateValidationException("Pipeline template is invalid", pipeline.errors as List<Map<String, Object>>)
     }
     return pipeline
   }