Merge branch 'master' into upstream-webhook-mitigations

Author: mergify[bot]

gradle.properties

@@ -1,5 +1,5 @@
 fiatVersion=1.50.0
-korkVersion=7.239.0
+korkVersion=7.240.0
 kotlinVersion=1.6.21
 org.gradle.parallel=true
 org.gradle.jvmargs=-Xmx4g

orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/job/WaitOnJobCompletion.groovy

@@ -147,7 +147,11 @@ public class WaitOnJobCompletion implements CloudProviderAware, OverridableTimeo
       InputStream jobStream
       retrySupport.retry({
         jobStream = katoRestService.collectJob(appName, account, location, name).body.in()
-      }, 6, 5000, false) // retry for 30 seconds
+      },
+          configProperties.getJobStatusRetry().maxAttempts,
+          Duration.ofMillis(configProperties.getJobStatusRetry().getBackOffInMs()),
+          configProperties.getJobStatusRetry().exponentialBackoffEnabled
+      )
       Map job = objectMapper.readValue(jobStream, new TypeReference<Map>() {})
       outputs.jobStatus = job
 
@@ -165,24 +169,16 @@ public class WaitOnJobCompletion implements CloudProviderAware, OverridableTimeo
 
       if ((status == ExecutionStatus.SUCCEEDED) || (status == ExecutionStatus.TERMINAL)) {
         if (stage.context.propertyFile) {
-          Map<String, Object> properties = [:]
-          try {
-            retrySupport.retry({
-              properties = katoRestService.getFileContents(appName, account, location, name, stage.context.propertyFile)
-            }, 6, 5000, false) // retry for 30 seconds
-          } catch (Exception e) {
-            if (status == ExecutionStatus.SUCCEEDED) {
-              throw new ConfigurationException("Property File: ${stage.context.propertyFile} contents could not be retrieved. Error: " + e)
-            }
-            log.warn("failed to get file contents for ${appName}, account: ${account}, namespace: ${location}, " +
-                "manifest: ${name} from propertyFile: ${stage.context.propertyFile}. Error: ", e)
-          }
-
-          if (properties.size() == 0) {
-            if (status == ExecutionStatus.SUCCEEDED) {
-              throw new ConfigurationException("Expected properties file ${stage.context.propertyFile} but it was either missing, empty or contained invalid syntax")
-            }
-          } else if (properties.size() > 0) {
+          Map<String, Object> properties = getPropertyFileContents(
+              job,
+              appName,
+              status,
+              account,
+              location,
+              name,
+              stage.context.propertyFile as String)
+
+          if (properties.size() > 0) {
             outputs << properties
             outputs.propertyFileContents = properties
           }
@@ -251,4 +247,197 @@ public class WaitOnJobCompletion implements CloudProviderAware, OverridableTimeo
     }
     throw new JobFailedException(errorMessage)
   }
+
+  /**
+   * <p>this method attempts to get property file from clouddriver and then parses its contents. Depending
+   * on the job itself, it could be handled by any job provider in clouddriver. This method should only be
+   * called for jobs with ExecutionStatus as either SUCCEEDED or TERMINAL.
+   *
+   * <p>If property file contents could not be retrieved from clouddriver, then the error handling depends
+   * on the job's ExecutionStatus. If it is SUCCEEDED, then an exception is thrown. Otherwise, no exception
+   * is thrown since we don't want to mask the real reason behind the job failure.
+   *
+   * <p>If ExecutionStatus == SUCCEEDED, and especially for kubernetes run jobs, it can so happen that a user
+   * has configured the job spec to run 1 pod, have completions and parallelism == 1, and
+   * restartPolicy == Never. Despite that, kubernetes may end up running another pod as stated here:
+   * https://kubernetes.io/docs/concepts/workloads/controllers/job/#handling-pod-and-container-failures
+   * In such a scenario, it may so happen that two pods are created for that job. The first pod may still be
+   * around, such as in a PodInitializing state and the second pod could complete before the first one is
+   * terminated. This leads to the getFileContents() call failing, since under the covers, kubernetes job
+   * provider runs kubectl logs job/<jobName> command, which picks one out of the two pods to obtain the
+   * logs as seen here:
+   *
+   * <p>kubectl -n test logs job/test-run-job-5j2vl -c parser
+   * Found 2 pods, using pod/test-run-job-5j2vl-fj8hd
+   * Error from server (BadRequest): container "parser" in pod "test-run-job-5j2vl-fj8hd" is PodInitializing
+   *
+   * <p>That means, even if kubernetes and clouddriver marked the job as successful, since number of
+   * succeeded pods >= number of completions, the kubectl command shown above could still end using
+   * the failed pod for obtaining the logs.
+   *
+   * <p>To handle this case, if we get an error while making the getFileContents() call or if we don't receive
+   *  any properties, then for kubernetes jobs, we figure out if the job status has any pod with phase
+   *  SUCCEEDED. If we find such a pod, then we directly get the logs from this succeeded pod. Otherwise,
+   *  we throw an exception as before.
+   *
+   *  <p> we aren't handling the above case for ExecutionStatus == TERMINAL, because at that point, we wouldn't
+   *  know which pod to query for properties file contents. It could so happen that all the pods in such a job
+   *  have failed, then we would have to loop over each pod and see what it generated. Then if say, two pods
+   *  generated different property values for the same key, which one do we choose? Bearing this complexity
+   *  in mind, and knowing that for succeeded jobs, this solution actually helps prevent a pipeline failure,
+   *  we are limiting this logic to succeeded jobs only for now.
+   *
+   * @param job - job status returned by clouddriver
+   * @param appName - application name where the job is run
+   * @param status - Execution status of the job. Should either be SUCCEEDED or TERMINAL
+   * @param account - account under which this job is run
+   * @param location - where this job is run
+   * @param name - name of the job
+   * @param propertyFile - file name to query from the job
+   * @return map of property file contents
+   */
+  private Map<String, Object> getPropertyFileContents(
+      Map job,
+      String appName,
+      ExecutionStatus status,
+      String account,
+      String location,
+      String name,
+      String propertyFile
+  ) {
+    Map<String, Object> properties = [:]
+    try {
+      retrySupport.retry({
+        properties = katoRestService.getFileContents(appName, account, location, name, propertyFile)
+      },
+          configProperties.getFileContentRetry().maxAttempts,
+          Duration.ofMillis(configProperties.getFileContentRetry().getBackOffInMs()),
+          configProperties.getFileContentRetry().exponentialBackoffEnabled
+      )
+    } catch (Exception e) {
+      log.warn("Error occurred while retrieving property file contents from job: ${name}" +
+          " in application: ${appName}, in account: ${account}, location: ${location}," +
+          " using propertyFile: ${propertyFile}. Error: ", e
+      )
+
+      // For succeeded kubernetes jobs, let's try one more time to get property file contents.
+      if (status == ExecutionStatus.SUCCEEDED) {
+        properties = getPropertyFileContentsForSucceededKubernetesJob(
+            job,
+            appName,
+            account,
+            location,
+            propertyFile
+        )
+        if (properties.size() == 0) {
+          // since we didn't get any properties, we fail with this exception
+          throw new ConfigurationException("Expected properties file: ${propertyFile} in " +
+              "job: ${name}, application: ${appName}, location: ${location}, account: ${account} " +
+              "but it was either missing, empty or contained invalid syntax. Error: ${e}")
+        }
+      }
+    }
+
+    if (properties.size() == 0) {
+      log.warn("Could not parse propertyFile: ${propertyFile} in job: ${name}" +
+          " in application: ${appName}, in account: ${account}, location: ${location}." +
+          " It is either missing, empty or contains invalid syntax"
+      )
+
+      // For succeeded kubernetes jobs, let's try one more time to get property file contents.
+      if (status == ExecutionStatus.SUCCEEDED) {
+        // let's try one more time to get properties from a kubernetes pod
+        properties = getPropertyFileContentsForSucceededKubernetesJob(
+            job,
+            appName,
+            account,
+            location,
+            propertyFile
+        )
+        if (properties.size() == 0) {
+          // since we didn't get any properties, we fail with this exception
+          throw new ConfigurationException("Expected properties file: ${propertyFile} in " +
+              "job: ${name}, application: ${appName}, location: ${location}, account: ${account} " +
+              "but it was either missing, empty or contained invalid syntax")
+        }
+      }
+    }
+    return properties
+  }
+
+  /**
+   * This method is supposed to be called from getPropertyFileContents(). This is only applicable for
+   * Kubernetes jobs. It finds a successful pod in the job and directly queries it for property file
+   * contents.
+   *
+   * <p>It is meant to handle the following case:
+   *
+   * <p> if ExecutionStatus == SUCCEEDED, and especially for kubernetes run jobs, it can so happen that a
+   * user has configured the job spec to run 1 pod, have completions and parallelism == 1, and
+   * restartPolicy == Never. Despite that, kubernetes may end up running another pod as stated here:
+   * https://kubernetes.io/docs/concepts/workloads/controllers/job/#handling-pod-and-container-failures
+   * In such a scenario, it may so happen that two pods are created for that job. The first pod may still be
+   * around, such as in a PodInitializing state and the second pod could complete before the first one is
+   * terminated. This leads to the getFileContents() call failing, since under the covers, kubernetes job
+   * provider runs kubectl logs job/<jobName> command, which picks one out of the two pods to obtain the
+   * logs as seen here:
+   *
+   * <p>kubectl -n test logs job/test-run-job-5j2vl -c parser
+   * Found 2 pods, using pod/test-run-job-5j2vl-fj8hd
+   * Error from server (BadRequest): container "parser" in pod "test-run-job-5j2vl-fj8hd" is PodInitializing
+   *
+   * <p>That means, even if kubernetes and clouddriver marked the job as successful, since number of
+   * succeeded pods >= number of completions, the kubectl command shown above could still end using
+   * the failed pod for obtaining the logs.
+   *
+   * <p>To handle this case, if we get an error while making the getFileContents() call or if we don't receive
+   * any properties, then for kubernetes jobs, we figure out if the job status has any pod with phase
+   * SUCCEEDED. If we find such a pod, then we directly get the logs from this succeeded pod. Otherwise,
+   * we throw an exception as before.
+   *
+   * <p>To keep it simple, and not worry about how to deal with property file
+   * contents obtained from various successful pods in a job, if that may happen, we simply query the first
+   * successful pod in that job.
+   *
+   * @param job - job status returned by clouddriver
+   * @param appName - application in which this job is run
+   * @param account - account under which this job is run
+   * @param namespace - where this job is run
+   * @param propertyFile - file name to query from the job
+   * @return map of property file contents
+   */
+  private Map<String, Object> getPropertyFileContentsForSucceededKubernetesJob(
+      Map job,
+      String appName,
+      String account,
+      String namespace,
+      String propertyFile
+  ) {
+    Map<String, Object> properties = [:]
+    if (job.get("provider", "unknown") == "kubernetes") {
+      Optional<Map> succeededPod = job.get("pods", [])
+          .stream()
+          .filter({ Map pod -> pod.get("status", [:]).get("phase", "Running") == "Succeeded"
+          })
+          .findFirst()
+
+      if (succeededPod.isPresent()) {
+        String podName = (succeededPod.get() as Map).get("name")
+        retrySupport.retry({
+          properties = katoRestService.getFileContentsFromKubernetesPod(
+              appName,
+              account,
+              namespace,
+              podName,
+              propertyFile
+          )
+        },
+            configProperties.getFileContentRetry().maxAttempts,
+            Duration.ofMillis(configProperties.getFileContentRetry().getBackOffInMs()),
+            configProperties.getFileContentRetry().exponentialBackoffEnabled
+        )
+      }
+    }
+    return properties
+  }
 }

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/DelegatingKatoRestService.java

@@ -71,6 +71,13 @@ public Map<String, Object> getFileContents(
     return getService().getFileContents(app, account, region, id, fileName);
   }
 
+  @Override
+  public Map<String, Object> getFileContentsFromKubernetesPod(
+      String app, String account, String namespace, String podName, String fileName) {
+    return getService()
+        .getFileContentsFromKubernetesPod(app, account, namespace, podName, fileName);
+  }
+
   @Override
   public Task lookupTask(String id) {
     return getService().lookupTask(id);

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/KatoRestService.java

@@ -73,6 +73,14 @@ Map<String, Object> getFileContents(
       @Path("id") String id,
       @Path("fileName") String fileName);
 
+  @GET("/applications/{app}/kubernetes/pods/{account}/{namespace}/{podName}/{fileName}")
+  Map<String, Object> getFileContentsFromKubernetesPod(
+      @Path("app") String app,
+      @Path("account") String account,
+      @Path("namespace") String namespace,
+      @Path("podName") String podName,
+      @Path("fileName") String fileName);
+
   /**
    * This should _only_ be called if there is a problem retrieving the Task from
    * CloudDriverTaskStatusService (ie. a clouddriver replica).

orca-clouddriver/src/main/java/com/netflix/spinnaker/orca/clouddriver/config/TaskConfigurationProperties.java

@@ -47,6 +47,22 @@ public static class WaitOnJobCompletionTaskConfig {
      * Default or empty set means that no keys will be excluded.
      */
     private Set<String> excludeKeysFromOutputs = Set.of();
+
+    private Retries jobStatusRetry = new Retries();
+
+    private Retries fileContentRetry = new Retries();
+
+    @Data
+    public static class Retries {
+      // total number of attempts
+      int maxAttempts = 6;
+
+      // time in ms to wait before subsequent retry attempts
+      long backOffInMs = 5000;
+
+      // flag to enable exponential backoff
+      boolean exponentialBackoffEnabled = false;
+    }
   }
 
   @Data