fix(pipeline): allow mid-task spel evaluation to reference prior stag…

…e outputs (#3220) * chore(pipeline): add buildExecutionContext test to ContextParameterProcessorSpec * fix(pipeline): allow mid-task SpEL evaluation to reference prior stage outputs
spinnaker · Oct 9, 2019 · a5bd525 · a5bd525
1 parent 37a306a
commit a5bd525
Show file tree

Hide file tree

Showing 8 changed files with 64 additions and 32 deletions.
diff --git a/...main/groovy/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java b/...main/groovy/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java
@@ -119,7 +119,7 @@ public TaskResult execute(@Nonnull Stage stage) {
 
       overrides =
           contextParameterProcessor.process(
-              overrides, contextParameterProcessor.buildExecutionContext(stage, true), true);
+              overrides, contextParameterProcessor.buildExecutionContext(stage), true);
     }
 
     BakeManifestRequest request;

diff --git a/.../main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluator.java b/.../main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluator.java
@@ -128,9 +128,7 @@ public Result evaluate(Stage stage, ManifestContext context) {
         if (!context.isSkipExpressionEvaluation()) {
           manifestWrapper =
               contextParameterProcessor.process(
-                  manifestWrapper,
-                  contextParameterProcessor.buildExecutionContext(stage, true),
-                  true);
+                  manifestWrapper, contextParameterProcessor.buildExecutionContext(stage), true);
 
           if (manifestWrapper.containsKey("expressionEvaluationSummary")) {
             throw new IllegalStateException(

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/OptionalStageSupport.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/OptionalStageSupport.java
@@ -99,7 +99,7 @@ public boolean evaluate(Stage stage, ContextParameterProcessor contextParameterP
           contextParameterProcessor
               .process(
                   singletonMap("expression", format("${%s}", this.expression)),
-                  contextParameterProcessor.buildExecutionContext(stage, true),
+                  contextParameterProcessor.buildExecutionContext(stage),
                   true)
               .get("expression")
               .toString();

diff --git a/...e/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/ExpressionPreconditionTask.java b/...e/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/ExpressionPreconditionTask.java
@@ -55,7 +55,7 @@ public ExpressionPreconditionTask(ContextParameterProcessor contextParameterProc
     Map<String, Object> result =
         contextParameterProcessor.process(
             singletonMap("expression", "${" + stageData.expression + '}'),
-            contextParameterProcessor.buildExecutionContext(stage, true),
+            contextParameterProcessor.buildExecutionContext(stage),
             true);
 
     String expression = result.get("expression").toString();

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactResolver.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactResolver.java
@@ -156,7 +156,7 @@ public ArtifactResolver(
 
     Map<String, Object> evaluatedBoundArtifactMap =
         contextParameterProcessor.process(
-            boundArtifactMap, contextParameterProcessor.buildExecutionContext(stage, true), true);
+            boundArtifactMap, contextParameterProcessor.buildExecutionContext(stage), true);
 
     return objectMapper.convertValue(evaluatedBoundArtifactMap, Artifact.class);
   }

diff --git a/...ore/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java b/...ore/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java
@@ -99,11 +99,9 @@ public Map<String, Object> process(
     return result;
   }
 
-  public Map<String, Object> buildExecutionContext(Stage stage, boolean includeStageContext) {
+  public StageContext buildExecutionContext(Stage stage) {
     Map<String, Object> augmentedContext = new HashMap<>();
-    if (includeStageContext) {
-      augmentedContext.putAll(stage.getContext());
-    }
+    augmentedContext.putAll(stage.getContext());
     if (stage.getExecution().getType() == PIPELINE) {
       augmentedContext.put(
           "trigger",
@@ -112,7 +110,7 @@ public Map<String, Object> buildExecutionContext(Stage stage, boolean includeSta
       augmentedContext.put("execution", stage.getExecution());
     }
 
-    return augmentedContext;
+    return new StageContext(stage, augmentedContext);
   }
 
   public static boolean containsExpression(String value) {

diff --git a/...test/groovy/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessorSpec.groovy b/...test/groovy/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessorSpec.groovy
@@ -803,7 +803,7 @@ class ContextParameterProcessorSpec extends Specification {
     }
 
     def stage = pipe.stageByRef("2")
-    def ctx = contextParameterProcessor.buildExecutionContext(stage, true)
+    def ctx = contextParameterProcessor.buildExecutionContext(stage)
 
     when:
     def result = contextParameterProcessor.process(stage.context, ctx, true)
@@ -825,7 +825,7 @@ class ContextParameterProcessorSpec extends Specification {
     }
 
     def stage = pipe.stageByRef("1")
-    def ctx = contextParameterProcessor.buildExecutionContext(stage, true)
+    def ctx = contextParameterProcessor.buildExecutionContext(stage)
 
     when:
     def result = contextParameterProcessor.process(stage.context, ctx, true)
@@ -854,7 +854,7 @@ class ContextParameterProcessorSpec extends Specification {
     pipe.setAuthentication(new Execution.AuthenticationDetails('joeyjoejoejuniorshabadoo@host.net'))
 
     def stage = pipe.stages.find { it.name == "Wait1" }
-    def ctx = contextParameterProcessor.buildExecutionContext(stage, true)
+    def ctx = contextParameterProcessor.buildExecutionContext(stage)
 
     when:
     def result = contextParameterProcessor.process(stage.context, ctx, true)
@@ -881,7 +881,7 @@ class ContextParameterProcessorSpec extends Specification {
 
     and:
     def stage = pipe.stages.find { it.type == "bake" }
-    def ctx = contextParameterProcessor.buildExecutionContext(stage, true)
+    def ctx = contextParameterProcessor.buildExecutionContext(stage)
 
     when:
     def result = contextParameterProcessor.process(stage.context, ctx, true)
@@ -911,7 +911,7 @@ class ContextParameterProcessorSpec extends Specification {
 
     and:
     def stage = pipe.stageByRef("1")
-    def ctx = contextParameterProcessor.buildExecutionContext(stage, true)
+    def ctx = contextParameterProcessor.buildExecutionContext(stage)
 
     when:
     def result = contextParameterProcessor.process(stage.context, ctx, true)
@@ -941,6 +941,54 @@ class ContextParameterProcessorSpec extends Specification {
     [branch:"hello"] || "hello"
   }
 
+  @Unroll
+  def "Correctly evaluates expressions that refer to outputs of prior stages"() {
+    given:
+    def execution = pipeline {
+      stage {
+        type = "evaluateVariables"
+        name = "Evaluate namespace"
+        refId = "1"
+        status = SUCCEEDED
+        outputs.putAll(
+          keyA: "valueA",
+          keyB: "valueB"
+        )
+      }
+      stage {
+        type = "deployManifest"
+        name = "Deploy manifest"
+        refId = "2"
+        requisiteStageRefIds = ["1"]
+        context.putAll(
+          manifests: [
+            [
+              kind: 'ReplicaSet',
+              name: '${keyA}',
+              namespace: '${keyB}'
+            ]
+
+          ]
+        )
+      }
+    }
+
+    def stage = execution.stageByRef("2")
+    def ctx = contextParameterProcessor.buildExecutionContext(stage)
+
+    when:
+    def result = contextParameterProcessor.process(stage.context, ctx, true)
+
+    then:
+    result.manifests == [
+      [
+        kind: 'ReplicaSet',
+        name: 'valueA',
+        namespace: 'valueB'
+      ]
+    ]
+  }
+
   static escapeExpression(String expression) {
     return ExpressionTransform.escapeSimpleExpression(expression)
   }

diff --git a/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt b/orca-queue/src/main/kotlin/com/netflix/spinnaker/orca/q/handler/ExpressionAware.kt
@@ -17,12 +17,10 @@
 package com.netflix.spinnaker.orca.q.handler
 
 import com.fasterxml.jackson.databind.ObjectMapper
-import com.fasterxml.jackson.module.kotlin.convertValue
 import com.netflix.spinnaker.kork.expressions.ExpressionEvaluationSummary
 import com.netflix.spinnaker.orca.exceptions.ExceptionHandler
 import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper
 import com.netflix.spinnaker.orca.pipeline.expressions.PipelineExpressionEvaluator
-import com.netflix.spinnaker.orca.pipeline.model.Execution
 import com.netflix.spinnaker.orca.pipeline.model.Execution.ExecutionType.PIPELINE
 import com.netflix.spinnaker.orca.pipeline.model.Stage
 import com.netflix.spinnaker.orca.pipeline.model.StageContext
@@ -48,6 +46,7 @@ interface ExpressionAware {
     val evalSummary = ExpressionEvaluationSummary()
     val processed = processEntries(this, evalSummary)
     val execution = execution
+    val stage = this
     this.context = object : MutableMap<String, Any?> by processed {
       override fun get(key: String): Any? {
         if (execution.type == PIPELINE) {
@@ -63,7 +62,7 @@ interface ExpressionAware {
         val result = processed[key]
 
         if (result is String && ContextParameterProcessor.containsExpression(result)) {
-          val augmentedContext = processed.augmentContext(execution)
+          val augmentedContext = contextParameterProcessor.buildExecutionContext(stage)
           return contextParameterProcessor.process(mapOf(key to result), augmentedContext, true)[key]
         }
 
@@ -130,23 +129,12 @@ interface ExpressionAware {
   private fun processEntries(stage: Stage, summary: ExpressionEvaluationSummary): StageContext =
     StageContext(stage, contextParameterProcessor.process(
       stage.context,
-      (stage.context as StageContext).augmentContext(stage.execution),
+      contextParameterProcessor.buildExecutionContext(stage),
       true,
       summary
     )
     )
 
-  // TODO (mvulfson): Ideally, we opt out of this method and use ContextParameterProcessor.buildExecutionContext
-  // but that doesn't generate StageContext preventing us from doing recursive lookups... An investigation for another day
-  private fun StageContext.augmentContext(execution: Execution): StageContext =
-    if (execution.type == PIPELINE) {
-      this + mapOf(
-        "trigger" to mapper.convertValue<Map<String, Any>>(execution.trigger),
-        "execution" to execution)
-    } else {
-      this
-    }
-
   private operator fun StageContext.plus(map: Map<String, Any?>): StageContext =
     StageContext(this).apply { putAll(map) }
 }