fix(artifacts): Automated triggers with artifact constraints are broken if you have 2 or more of the same type (#4579)

* refactor(artifacts): partially reverting #4397

* refactor(artifacts): reverted #4489

* refactor(artifacts): reverted #4526

* test(artifacts): added test when parent pipeline does not provide expected artifacts

* test(artifacts): resolve artifacts for default and prior artifacts

---------

Co-authored-by: Cameron Motevasselani <[email protected]>

Author: nemesisOsorio

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

@@ -28,31 +28,26 @@
 import com.netflix.spinnaker.kork.annotations.NonnullByDefault;
 import com.netflix.spinnaker.kork.artifacts.model.Artifact;
 import com.netflix.spinnaker.kork.artifacts.model.ExpectedArtifact;
-import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException;
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
 import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
 import com.netflix.spinnaker.orca.pipeline.model.StageContext;
 import com.netflix.spinnaker.orca.pipeline.model.StageExecutionImpl;
 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;
 import javax.annotation.CheckReturnValue;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
-import org.apache.commons.lang3.ObjectUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -144,31 +139,7 @@ private List<Artifact> getAllArtifacts(
         contextParameterProcessor.process(
             boundArtifactMap, contextParameterProcessor.buildExecutionContext(stage), true);
 
-    Artifact evaluatedArtifact =
-        objectMapper.convertValue(evaluatedBoundArtifactMap, Artifact.class);
-    return getBoundInlineArtifact(evaluatedArtifact, stage.getExecution())
-        .orElse(evaluatedArtifact);
-  }
-
-  private Optional<Artifact> getBoundInlineArtifact(
-      @Nullable Artifact artifact, PipelineExecution execution) {
-    if (ObjectUtils.anyNull(
-        artifact, execution.getTrigger(), execution.getTrigger().getArtifacts())) {
-      return Optional.empty();
-    }
-    try {
-      ExpectedArtifact expectedArtifact =
-          ExpectedArtifact.builder().matchArtifact(artifact).build();
-      return ArtifactResolver.getInstance(execution.getTrigger().getArtifacts(), true)
-          .resolveExpectedArtifacts(List.of(expectedArtifact))
-          .getResolvedExpectedArtifacts()
-          .stream()
-          .findFirst()
-          .flatMap(this::getBoundArtifact);
-    } catch (InvalidRequestException e) {
-      log.debug("Could not match inline artifact with trigger bound artifacts", e);
-      return Optional.empty();
-    }
+    return objectMapper.convertValue(evaluatedBoundArtifactMap, Artifact.class);
   }
 
   public @Nullable Artifact getBoundArtifactForId(StageExecution stage, @Nullable String id) {
@@ -230,38 +201,10 @@ public List<Artifact> getArtifactsForPipelineIdWithoutStageRef(
         .orElse(Collections.emptyList());
   }
 
-  private List<String> getExpectedArtifactIdsFromMap(Map<String, Object> trigger) {
-    List<String> expectedArtifactIds = (List<String>) trigger.get("expectedArtifactIds");
-    return (expectedArtifactIds != null) ? expectedArtifactIds : emptyList();
-  }
-
   public void resolveArtifacts(Map pipeline) {
     Map<String, Object> trigger = (Map<String, Object>) pipeline.get("trigger");
-    List<?> triggers = Optional.ofNullable((List<?>) pipeline.get("triggers")).orElse(emptyList());
-    Set<String> expectedArtifactIdsListConcat =
-        new HashSet<>(getExpectedArtifactIdsFromMap(trigger));
-
-    // 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()
-        .map(it -> (Map<String, Object>) 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<String> expectedArtifactIds = new ArrayList<>(expectedArtifactIdsListConcat);
+    List<?> expectedArtifactIds =
+        (List<?>) trigger.getOrDefault("expectedArtifactIds", emptyList());
     ImmutableList<ExpectedArtifact> expectedArtifacts =
         Optional.ofNullable((List<?>) pipeline.get("expectedArtifacts"))
             .map(Collection::stream)

orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/util/ArtifactUtilsSpec.groovy

@@ -19,7 +19,6 @@ 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
@@ -82,29 +81,6 @@ class ArtifactUtilsSpec extends Specification {
     artifact.name == 'build/libs/my-jar-100.jar'
   }
 
-  def "should bind stage-inlined artifacts to trigger artifacts"() {
-    setup:
-    def execution = pipeline {
-      stage {
-        name = "upstream stage"
-        type = "stage1"
-        refId = "1"
-      }
-    }
-
-    execution.trigger = new DefaultTrigger('manual')
-    execution.trigger.artifacts.add(Artifact.builder().type('http/file').name('build/libs/my-jar-100.jar').build())
-
-    when:
-    def artifact = makeArtifactUtils().getBoundArtifactForStage(execution.stages[0], null, Artifact.builder()
-        .type('http/file')
-        .name('build/libs/my-jar-\\d+.jar')
-        .build())
-
-    then:
-    artifact.name == 'build/libs/my-jar-100.jar'
-  }
-
   def "should find upstream artifacts in small pipeline"() {
     when:
     def desired = execution.getStages().find { it.name == "desired" }
@@ -516,81 +492,118 @@ class ArtifactUtilsSpec extends Specification {
     initialArtifacts == finalArtifacts
   }
 
-  def "should find artifact if triggers is present in pipeline"() {
+  def "resolve expected artifact using default artifact"() {
     given:
-    def defaultArtifact = Artifact.builder()
-        .customKind(true)
+    def matchArtifact = Artifact
+        .builder()
+        .name("my-artifact")
+        .artifactAccount("embedded-artifact")
+        .type("embedded/base64")
         .build()
-
-    def matchArtifact = Artifact.builder()
-        .name("my-pipeline-artifact")
+    def defaultArtifact = Artifact
+        .builder()
+        .name("default-artifact")
+        .artifactAccount("embedded-artifact")
         .type("embedded/base64")
-        .reference("aGVsbG8gd29ybGQK")
+        .reference("bmVtZXNpcwo=")
         .build()
-
-    def expectedArtifact = ExpectedArtifact.builder()
-        .usePriorArtifact(false)
-        .useDefaultArtifact(false)
-        .id("my-id")
-        .defaultArtifact(defaultArtifact)
+    def expectedArtifact = ExpectedArtifact
+        .builder()
         .matchArtifact(matchArtifact)
+        .defaultArtifact(defaultArtifact)
+        .useDefaultArtifact(true)
         .build()
 
-    def expectedArtifact2 = ExpectedArtifact.builder()
-        .usePriorArtifact(false)
-        .useDefaultArtifact(false)
-        .id("my-id-2")
-        .defaultArtifact(defaultArtifact)
+    def pipeline = [
+        id               : "01HE3GXEJX05143Y7JSGTRRB40",
+        trigger          : [
+            type: "manual",
+            // not passing artifacts in trigger
+        ],
+        expectedArtifacts: [expectedArtifact],
+    ]
+    def artifactUtils = makeArtifactUtils()
+
+    when:
+    artifactUtils.resolveArtifacts(pipeline)
+    List<ExpectedArtifact> resolvedArtifacts = objectMapper.convertValue(
+        pipeline.trigger.resolvedExpectedArtifacts,
+        new TypeReference<List<ExpectedArtifact>>() {}
+    )
+
+    then:
+    pipeline.trigger.artifacts.size() == 1
+    pipeline.trigger.expectedArtifacts.size() == 1
+    pipeline.trigger.resolvedExpectedArtifacts.size() == 1
+    resolvedArtifacts*.getBoundArtifact() == [defaultArtifact]
+  }
+
+  def "resolve expected artifact using prior artifact"() {
+    given:
+    def artifactName = "my-artifact-name"
+    def priorArtifact = Artifact
+        .builder()
+        .name(artifactName)
+        .artifactAccount("embedded-artifact")
+        .type("embedded/base64")
+        .reference("b3NvcmlvCg==")
+        .build()
+
+    def pipelineId = "01HE3GXEJX05143Y7JSGTRRB41"
+    def priorExecution = pipeline {
+      id:
+      pipelineId
+      status:
+      ExecutionStatus.SUCCEEDED
+      stage {
+        refId = "1"
+        outputs.artifacts = [priorArtifact]
+      }
+    }
+
+    ExecutionRepository.ExecutionCriteria criteria = new ExecutionRepository.ExecutionCriteria();
+    criteria.setPageSize(1);
+    criteria.setSortType(ExecutionRepository.ExecutionComparator.START_TIME_OR_ID);
+
+    def executionRepositoryMock = Mock(ExecutionRepository) {
+      retrievePipelinesForPipelineConfigId(pipelineId, criteria) >> Observable.just(priorExecution)
+    }
+
+    def matchArtifact = Artifact
+        .builder()
+        .name(artifactName)
+        .artifactAccount("embedded-artifact")
+        .type("embedded/base64")
+        .build()
+    def expectedArtifact = ExpectedArtifact
+        .builder()
         .matchArtifact(matchArtifact)
+        .usePriorArtifact(true)
         .build()
 
     def pipeline = [
-        "id": "abc",
-        "stages": [
-            stage {
-              expectedArtifacts: [expectedArtifact]
-              inputArtifacts: [
-                  "id": "my-id"
-              ]
-            }
+        id               : pipelineId,
+        trigger          : [
+            type: "manual",
+            // not passing artifacts in trigger
         ],
-        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"
-            ]
-        ]
+        expectedArtifacts: [expectedArtifact],
     ]
 
-    def pipelineMap = getObjectMapper().convertValue(pipeline, Map.class)
+    def artifactUtils = makeArtifactUtilsWithStub(executionRepositoryMock)
+
     when:
-     makeArtifactUtils().resolveArtifacts(pipelineMap)
+    artifactUtils.resolveArtifacts(pipeline)
+    List<ExpectedArtifact> resolvedArtifacts = objectMapper.convertValue(
+        pipeline.trigger.resolvedExpectedArtifacts,
+        new TypeReference<List<ExpectedArtifact>>() {}
+    )
 
     then:
-    pipelineMap.trigger.resolvedExpectedArtifacts.size() == 1
+    pipeline.trigger.artifacts.size() == 1
+    pipeline.trigger.expectedArtifacts.size() == 1
+    pipeline.trigger.resolvedExpectedArtifacts.size() == 1
+    resolvedArtifacts*.getBoundArtifact() == [priorArtifact]
   }
 
   private List<Artifact> extractTriggerArtifacts(Map<String, Object> trigger) {

orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy

@@ -87,15 +87,6 @@ class DependentPipelineStarter implements ApplicationContextAware {
       it.expectedArtifactIds ?: []
     }
 
-    // we are following a similar approach as triggers above
-    // expectedArtifacts can be used in triggers and stages
-    // for now we identified DeployManifestStage
-    // in ResolveDeploySourceManifestTask using ManifestEvaluator.getRequiredArtifacts
-    def requiredArtifactIds = pipelineConfig.get("stages", []).collectMany {
-      it.requiredArtifactIds ?: []
-    }
-    expectedArtifactIds.addAll(requiredArtifactIds)
-
     pipelineConfig.trigger = [
       type                 : "pipeline",
       user                 : authenticationDetails?.user ?: user ?: "[anonymous]",