fix(exceptions): add exception handler for exceptional situations (sp…

…innaker#3845)

* fix(exceptions): add exceptionhandler for non-excpetional situations

When a user submits a bad pipeline e.g.
* with jenkins trigger that has an invalid properties file or job that doesn't exist)
* with a broken pipeline template

we don't want to 500 because:
a) the calling service (echo in most cases here) will keep retrying which makes no sense
b) it's an erroneous HTTP 500 metric that is noise

* fix(exceptions): fix various exception issues
1. Don't log task failures on `UserException`
2. Don't fail retrieving properties file if the build wasn't started

* fixup! Merge branch 'master' into mark/exceptionhandlers

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java

@@ -24,6 +24,7 @@
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spectator.api.Registry;
+import com.netflix.spinnaker.kork.exceptions.UserException;
 import com.netflix.spinnaker.kork.web.exceptions.ValidationException;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType;
@@ -181,7 +182,15 @@ private PipelineExecution handleStartupFailure(PipelineExecution execution, Thro
       }
     }
 
-    log.error("Failed to start {} {}", execution.getType(), execution.getId(), failure);
+    if (failure instanceof UserException) {
+      log.warn(
+          "Failed to start {} {} due to user error or misconfiguration",
+          execution.getType(),
+          execution.getId(),
+          failure);
+    } else {
+      log.error("Failed to start {} {}", execution.getType(), execution.getId(), failure);
+    }
     executionRepository.updateStatus(execution.getType(), execution.getId(), status);
     executionRepository.cancel(execution.getType(), execution.getId(), canceledBy, reason);
     return executionRepository.retrieve(execution.getType(), execution.getId());

orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTask.java

@@ -16,6 +16,8 @@
 
 package com.netflix.spinnaker.orca.igor.tasks;
 
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException;
+import com.netflix.spinnaker.kork.exceptions.UserException;
 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;
@@ -41,14 +43,26 @@ public class GetBuildPropertiesTask extends RetryableIgorTask<CIStageDefinition>
       return TaskResult.SUCCEEDED;
     }
 
+    if ((stageDefinition.getBuildNumber() == null)) {
+      throw new UserException(
+          "Can't retrieve property file because the build number is not available, check for build start failures");
+    }
+
+    if ((stageDefinition.getPropertyFile() == null)
+        || (stageDefinition.getMaster() == null)
+        || (stageDefinition.getJob() == null)) {
+      throw new UserException(
+          "Can't retrieve property file: propertyFile, master, and job can't be null, check for build start failures");
+    }
+
     Map<String, Object> properties =
         buildService.getPropertyFile(
             stageDefinition.getBuildNumber(),
             stageDefinition.getPropertyFile(),
             stageDefinition.getMaster(),
             stageDefinition.getJob());
     if (properties.size() == 0) {
-      throw new IllegalStateException(
+      throw new ConfigurationException(
           String.format(
               "Expected properties file %s but it was either missing, empty or contained invalid syntax",
               stageDefinition.getPropertyFile()));

orca-igor/src/main/groovy/com/netflix/spinnaker/orca/igor/tasks/RetryableIgorTask.java

@@ -89,10 +89,12 @@ private boolean isRetryable(RetrofitError retrofitError) {
       return true;
     }
 
-    int status = retrofitError.getResponse().getStatus();
-    if (status == 500 || status == 503) {
-      log.warn(String.format("Received HTTP %s response from igor, retrying...", status));
-      return true;
+    if (retrofitError.getResponse() != null) {
+      int status = retrofitError.getResponse().getStatus();
+      if (status == 500 || status == 503) {
+        log.warn(String.format("Received HTTP %s response from igor, retrying...", status));
+        return true;
+      }
     }
     return false;
   }

orca-igor/src/test/groovy/com/netflix/spinnaker/orca/igor/tasks/GetBuildPropertiesTaskSpec.groovy

@@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.igor.tasks
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.kork.artifacts.model.Artifact
+import com.netflix.spinnaker.kork.exceptions.ConfigurationException
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
 import com.netflix.spinnaker.orca.api.pipeline.TaskResult
 import com.netflix.spinnaker.orca.igor.BuildService
@@ -141,7 +142,7 @@ class GetBuildPropertiesTaskSpec extends Specification {
     task.execute(stage)
 
     then:
-    IllegalStateException e = thrown IllegalStateException
+    ConfigurationException e = thrown ConfigurationException
     e.message == "Expected properties file $PROPERTY_FILE but it was either missing, empty or contained invalid syntax"
   }
 

orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/RunTaskHandler.kt

@@ -170,8 +170,12 @@ class RunTaskHandler(
             } else if (e is TimeoutException && stage.context["markSuccessfulOnTimeout"] == true) {
               queue.push(CompleteTask(message, SUCCEEDED))
             } else {
-              if (e !is TimeoutException && e !is UserException) {
-                log.error("Error running ${message.taskType.simpleName} for ${message.executionType}[${message.executionId}]", e)
+              if (e !is TimeoutException) {
+                if (e is UserException) {
+                  log.warn("${message.taskType.simpleName} for ${message.executionType}[${message.executionId}] failed, likely due to user error", e)
+                } else {
+                  log.error("Error running ${message.taskType.simpleName} for ${message.executionType}[${message.executionId}]", e)
+                }
               }
 
               val status = stage.failureStatus(default = TERMINAL)

orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/OperationsController.groovy

@@ -23,8 +23,7 @@ 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.kork.exceptions.UserException
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution
 import com.netflix.spinnaker.orca.clouddriver.service.JobService
 import com.netflix.spinnaker.orca.exceptions.OperationFailedException
@@ -330,7 +329,7 @@ class OperationsController {
         buildInfo = buildService.getBuild(trigger.buildNumber, trigger.master, trigger.job)
       } catch (RetrofitError e) {
         if (e.response?.status == 404) {
-          throw new IllegalStateException("Build ${trigger.buildNumber} of ${trigger.master}/${trigger.job} not found")
+          throw new ConfigurationException("Build ${trigger.buildNumber} of ${trigger.master}/${trigger.job} not found")
         } else {
           throw new OperationFailedException("Failed to get build ${trigger.buildNumber} of ${trigger.master}/${trigger.job}", e)
         }
@@ -351,7 +350,7 @@ class OperationsController {
           )
         } catch (RetrofitError e) {
           if (e.response?.status == 404) {
-            throw new IllegalStateException("Expected properties file " + trigger.propertyFile + " (configured on trigger), but it was missing")
+            throw new ConfigurationException("Expected properties file " + trigger.propertyFile + " (configured on trigger), but it was missing")
           } else {
             throw new OperationFailedException("Failed to get properties file ${trigger.propertyFile}", e)
           }

orca-web/src/main/groovy/com/netflix/spinnaker/orca/controllers/V2PipelineTemplateController.java

@@ -25,10 +25,7 @@
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression;
-import org.springframework.web.bind.annotation.RequestBody;
-import org.springframework.web.bind.annotation.RequestMapping;
-import org.springframework.web.bind.annotation.RequestMethod;
-import org.springframework.web.bind.annotation.RestController;
+import org.springframework.web.bind.annotation.*;
 
 @RestController
 @RequestMapping(value = "/v2/pipelineTemplates")

orca-web/src/main/groovy/com/netflix/spinnaker/orca/exceptions/OperationFailedException.java

@@ -1,6 +1,8 @@
 package com.netflix.spinnaker.orca.exceptions;
 
-public class OperationFailedException extends RuntimeException {
+import com.netflix.spinnaker.kork.exceptions.IntegrationException;
+
+public class OperationFailedException extends IntegrationException {
   public OperationFailedException(String message) {
     super(message);
   }