From a5bd525b4701a0a869f13b7747d7a03cb2a49895 Mon Sep 17 00:00:00 2001 From: Maggie Neterval Date: Wed, 9 Oct 2019 09:55:50 -0400 Subject: [PATCH] fix(pipeline): allow mid-task spel evaluation to reference prior stage outputs (#3220) * chore(pipeline): add buildExecutionContext test to ContextParameterProcessorSpec * fix(pipeline): allow mid-task SpEL evaluation to reference prior stage outputs --- .../manifests/CreateBakeManifestTask.java | 2 +- .../tasks/manifest/ManifestEvaluator.java | 4 +- .../pipeline/model/OptionalStageSupport.java | 2 +- .../tasks/ExpressionPreconditionTask.java | 2 +- .../orca/pipeline/util/ArtifactResolver.java | 2 +- .../util/ContextParameterProcessor.java | 8 +-- .../util/ContextParameterProcessorSpec.groovy | 58 +++++++++++++++++-- .../orca/q/handler/ExpressionAware.kt | 18 +----- 8 files changed, 64 insertions(+), 32 deletions(-) diff --git a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java b/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java index 10e31ec882..f728de4977 100644 --- a/orca-bakery/src/main/groovy/com/netflix/spinnaker/orca/bakery/tasks/manifests/CreateBakeManifestTask.java +++ b/orca-bakery/src/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/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluator.java b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluator.java index 5414e8d745..427184c3b6 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/tasks/manifest/ManifestEvaluator.java +++ b/orca-clouddriver/src/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 index c4e5840434..66626378c0 100644 --- 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/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/ExpressionPreconditionTask.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/ExpressionPreconditionTask.java index 48e464cf6f..a92762d043 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/ExpressionPreconditionTask.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/tasks/ExpressionPreconditionTask.java @@ -55,7 +55,7 @@ public ExpressionPreconditionTask(ContextParameterProcessor contextParameterProc Map 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 index b7b2143f48..3209b3bef9 100644 --- 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 evaluatedBoundArtifactMap = contextParameterProcessor.process( - boundArtifactMap, contextParameterProcessor.buildExecutionContext(stage, true), true); + boundArtifactMap, contextParameterProcessor.buildExecutionContext(stage), true); return objectMapper.convertValue(evaluatedBoundArtifactMap, Artifact.class); } diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java index fc04b79dd5..86c404ffcd 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessor.java @@ -99,11 +99,9 @@ public Map process( return result; } - public Map buildExecutionContext(Stage stage, boolean includeStageContext) { + public StageContext buildExecutionContext(Stage stage) { Map 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 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/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessorSpec.groovy b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessorSpec.groovy index fcd8ca7445..0d4d69b4a2 100644 --- a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ContextParameterProcessorSpec.groovy +++ b/orca-core/src/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 index 7b034eafa1..f7078ce5e1 100644 --- 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 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>(execution.trigger), - "execution" to execution) - } else { - this - } - private operator fun StageContext.plus(map: Map): StageContext = StageContext(this).apply { putAll(map) } }