From d1a6960d745dc31089c0c6eea0e6adef83136353 Mon Sep 17 00:00:00 2001 From: Benevolent Benjamin Powell Date: Fri, 15 Sep 2023 03:14:50 -0500 Subject: [PATCH 1/3] fix(artifacts): Resolving is broken in multiple places 8df68b79cf1 broke custom triggers to Spinnaker integration. Prior to this commit if `getBoundArtifactForStage` failed to find a bound artifact, resolution would happen much later by utilizing the `triggers` field in stages. However, `getBoundArtifactForStage` now throws an exception, which causes custom triggers to fail prematurely. Also if a custom trigger calls orchestrate directly and any pipeline contains `getBoundArtifactForStage`, this will throw an exception and prevent any resolution. Signed-off-by: benjamin-j-powell --- .../orca/pipeline/util/ArtifactUtils.java | 42 +++++++++- .../pipeline/util/ArtifactUtilsSpec.groovy | 78 +++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java index f57085a5d2..01b0bf1cbf 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java @@ -36,13 +36,16 @@ import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository; import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionCriteria; import java.io.IOException; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -227,10 +230,45 @@ public List getArtifactsForPipelineIdWithoutStageRef( .orElse(Collections.emptyList()); } + private List getExpectedArtifactIdsFromMap(Map trigger) { + return Optional.ofNullable((List) trigger.get("expectedArtifactIds")) + .orElse(emptyList()); + } + public void resolveArtifacts(Map pipeline) { Map trigger = (Map) pipeline.get("trigger"); - List expectedArtifactIds = - (List) trigger.getOrDefault("expectedArtifactIds", emptyList()); + List triggers = Optional.ofNullable((List) pipeline.get("triggers")).orElse(emptyList()); + Set expectedArtifactIdsListConcat = + new HashSet<>(getExpectedArtifactIdsFromMap(trigger)); + + // Due to 8df68b79cf1 getBoundArtifactForStage now does resolution which can + // potentially return an empty list or null back, which will throw an + // exception. Before this commit, when getBoundArtifactForStage was called, + // the method would just retrieve the bound artifact from the stage context, + // and return it as an ExpectedArtifact type instead of a map. "Resolution" + // would happen later in the DependentPipelineStarter class which handles + // the overall pipeline triggers case which we have now added here. + // + // Another case is when any custom trigger occurs, and it runs the + // orchestrate endpoint and if any pipeline stage uses + // getBoundArtifactForStage then this will throw an exception, instead of + // returning the bound artifact which will be later be added as an resolved + // artifact in the context. A good example of this is the + // CreateBakeManifestTask. + // + // reference: https://github.com/spinnaker/orca/pull/4397 + triggers.stream() + .map(it -> (Map) it) + // This filter prevents multiple triggers from adding its + // expectedArtifactIds unless it is the expected trigger type that was + // triggered + // + // reference: https://github.com/spinnaker/orca/pull/4322 + .filter(it -> trigger.getOrDefault("type", "").equals(it.get("type"))) + .map(this::getExpectedArtifactIdsFromMap) + .forEach(expectedArtifactIdsListConcat::addAll); + + final List expectedArtifactIds = new ArrayList<>(expectedArtifactIdsListConcat); ImmutableList expectedArtifacts = Optional.ofNullable((List) pipeline.get("expectedArtifacts")) .map(Collection::stream) diff --git a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy index 97ff6240d8..e8fc88b65b 100644 --- a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy +++ b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy @@ -19,6 +19,7 @@ package com.netflix.spinnaker.orca.pipeline.util import com.fasterxml.jackson.core.type.TypeReference import com.fasterxml.jackson.databind.ObjectMapper +import com.netflix.spinnaker.kork.artifacts.ArtifactTypes import com.netflix.spinnaker.kork.artifacts.model.Artifact import com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus @@ -515,6 +516,83 @@ class ArtifactUtilsSpec extends Specification { initialArtifacts == finalArtifacts } + def "should find artifact if triggers is present in pipeline"() { + given: + def defaultArtifact = Artifact.builder() + .customKind(true) + .build() + + def matchArtifact = Artifact.builder() + .name("my-pipeline-artifact") + .type("embedded/base64") + .reference("aGVsbG8gd29ybGQK") + .build() + + def expectedArtifact = ExpectedArtifact.builder() + .usePriorArtifact(false) + .useDefaultArtifact(false) + .id("my-id") + .defaultArtifact(defaultArtifact) + .matchArtifact(matchArtifact) + .build() + + def expectedArtifact2 = ExpectedArtifact.builder() + .usePriorArtifact(false) + .useDefaultArtifact(false) + .id("my-id-2") + .defaultArtifact(defaultArtifact) + .matchArtifact(matchArtifact) + .build() + + def pipeline = [ + "id": "abc", + "stages": [ + stage { + expectedArtifacts: [expectedArtifact] + inputArtifacts: [ + "id": "my-id" + ] + } + ], + expectedArtifacts: [ + expectedArtifact + ], + trigger: [ + artifacts: [ + Artifact.builder() + .type(ArtifactTypes.EMBEDDED_BASE64.getMimeType()) + .name(matchArtifact.getName()) + .reference(matchArtifact.getReference()) + .build() + ], + type: "some-type" + ], + triggers: [ + [ + enabled: true, + expectedArtifactIds: [ + expectedArtifact.getId() + ], + type: "some-type" + ], + [ + enabled: true, + expectedArtifactIds: [ + expectedArtifact2.getId() + ], + type: "some-other-type" + ] + ] + ] + + def pipelineMap = getObjectMapper().convertValue(pipeline, Map.class) + when: + makeArtifactUtils().resolveArtifacts(pipelineMap) + + then: + pipelineMap.trigger.resolvedExpectedArtifacts.size() == 1 + } + private List extractTriggerArtifacts(Map trigger) { return objectMapper.convertValue(trigger.artifacts, new TypeReference>(){}); } From d4898e3ed3a272e4c7b75c3849312b524b8fdedf Mon Sep 17 00:00:00 2001 From: benjamin-j-powell Date: Mon, 18 Sep 2023 14:43:17 -0500 Subject: [PATCH 2/3] fixup! fix(artifacts): Resolving is broken in multiple places Signed-off-by: benjamin-j-powell --- .../netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java index 01b0bf1cbf..eba5ea0f97 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java @@ -231,8 +231,8 @@ public List getArtifactsForPipelineIdWithoutStageRef( } private List getExpectedArtifactIdsFromMap(Map trigger) { - return Optional.ofNullable((List) trigger.get("expectedArtifactIds")) - .orElse(emptyList()); + List expectedArtifactIds = (List) trigger.get("expectedArtifactIds"); + return (expectedArtifactIds != null) ? expectedArtifactIds : emptyList(); } public void resolveArtifacts(Map pipeline) { From 592570c3d09047868eed147f6e4d3e18c78f6da7 Mon Sep 17 00:00:00 2001 From: benjamin-j-powell Date: Tue, 19 Sep 2023 14:32:16 -0500 Subject: [PATCH 3/3] fixup! fix(artifacts): Resolving is broken in multiple places Signed-off-by: benjamin-j-powell --- .../orca/pipeline/util/ArtifactUtils.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java index eba5ea0f97..1fb5b059ea 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtils.java @@ -241,20 +241,13 @@ public void resolveArtifacts(Map pipeline) { Set expectedArtifactIdsListConcat = new HashSet<>(getExpectedArtifactIdsFromMap(trigger)); - // Due to 8df68b79cf1 getBoundArtifactForStage now does resolution which can - // potentially return an empty list or null back, which will throw an - // exception. Before this commit, when getBoundArtifactForStage was called, - // the method would just retrieve the bound artifact from the stage context, - // and return it as an ExpectedArtifact type instead of a map. "Resolution" - // would happen later in the DependentPipelineStarter class which handles - // the overall pipeline triggers case which we have now added here. - // - // Another case is when any custom trigger occurs, and it runs the - // orchestrate endpoint and if any pipeline stage uses - // getBoundArtifactForStage then this will throw an exception, instead of - // returning the bound artifact which will be later be added as an resolved - // artifact in the context. A good example of this is the - // CreateBakeManifestTask. + // Due to 8df68b79cf1 getBoundArtifactForStage now does resolution which + // can potentially return null artifact back, which will throw an exception + // for stages that expect a non-null value. Before this commit, when + // getBoundArtifactForStage was called, the method would just retrieve the + // bound artifact from the stage context, and return the appropriate + // artifact back. This change prevents tasks like CreateBakeManifestTask + // from working properly, if null is returned. // // reference: https://github.com/spinnaker/orca/pull/4397 triggers.stream()