From 4d166ca069f7f12af0c47641f06a99aaca889da7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Jervidalo?= Date: Thu, 3 Nov 2022 15:09:49 +0100 Subject: [PATCH 1/4] fix(artifacts): Expected Artifacts should be trigger specific When setting up multiple triggers with different expected artifacts, triggering the pipeline fails if _any_ defined expected artifact is missing, even if it is defined on another trigger. This fix filters the list of expected artifacts to make sure only the artifacts that are relevant for the current trigger is evaluated. --- .../orca/pipeline/util/ArtifactUtils.java | 3 +++ .../pipeline/util/ArtifactUtilsSpec.groovy | 27 +++++++++++++++++-- 2 files changed, 28 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 8790ce2ec8..4ae052f4e5 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 @@ -197,11 +197,14 @@ public List getArtifactsForPipelineIdWithoutStageRef( public void resolveArtifacts(Map pipeline) { Map trigger = (Map) pipeline.get("trigger"); + List expectedArtifactIds = + (List) trigger.getOrDefault("expectedArtifactIds", emptyList()); ImmutableList expectedArtifacts = Optional.ofNullable((List) pipeline.get("expectedArtifacts")) .map(Collection::stream) .orElse(Stream.empty()) .map(it -> objectMapper.convertValue(it, ExpectedArtifact.class)) + .filter(artifact -> expectedArtifactIds.contains(artifact.getId())) .collect(toImmutableList()); ImmutableSet receivedArtifacts = 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 98139bf346..6bdc59894a 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 @@ -369,11 +369,11 @@ class ArtifactUtilsSpec extends Specification { def "resolveArtifacts sets the bound artifact on an expected artifact"() { given: def matchArtifact = Artifact.builder().type("docker/.*").build() - def expectedArtifact = ExpectedArtifact.builder().matchArtifact(matchArtifact).build() + def expectedArtifact = ExpectedArtifact.builder().id("543ef192-82a2-4805-8d0c-827f2f976a1c").matchArtifact(matchArtifact).build() def receivedArtifact = Artifact.builder().name("my-artifact").type("docker/image").build() def pipeline = [ id: "abc", - trigger: [:], + trigger: ["expectedArtifactIds": ["543ef192-82a2-4805-8d0c-827f2f976a1c"]], expectedArtifacts: [expectedArtifact], receivedArtifacts: [receivedArtifact], ] @@ -390,6 +390,29 @@ class ArtifactUtilsSpec extends Specification { resolvedArtifacts.get(0).getBoundArtifact() == receivedArtifact } + def "resolveArtifacts ignores expected artifacts from unrelated triggers"() { + given: + def matchArtifact = Artifact.builder().type("docker/.*").build() + def expectedArtifact = ExpectedArtifact.builder().id("543ef192-82a2-4805-8d0c-827f2f976a1c").matchArtifact(matchArtifact).build() + def receivedArtifact = Artifact.builder().name("my-artifact").type("docker/image").build() + def pipeline = [ + id: "abc", + trigger: [:], + expectedArtifacts: [expectedArtifact], + receivedArtifacts: [receivedArtifact], + ] + def artifactUtils = makeArtifactUtils() + + when: + artifactUtils.resolveArtifacts(pipeline) + List resolvedArtifacts = objectMapper.convertValue( + pipeline.trigger.resolvedExpectedArtifacts, + new TypeReference>() {}) + + then: + resolvedArtifacts.size() == 0 + } + def "resolveArtifacts adds received artifacts to the trigger, skipping duplicates"() { given: def matchArtifact = Artifact.builder().name("my-pipeline-artifact").type("docker/.*").build() From bc27065c1330ee333164259a208552b4d85184fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Jervidalo?= Date: Thu, 3 Nov 2022 23:11:03 +0100 Subject: [PATCH 2/4] Fix tests in ArtifactUtilsSpec MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jørgen Jervidalo --- .../orca/pipeline/util/ArtifactUtilsSpec.groovy | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) 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 6bdc59894a..b034a3202e 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 @@ -393,12 +393,16 @@ class ArtifactUtilsSpec extends Specification { def "resolveArtifacts ignores expected artifacts from unrelated triggers"() { given: def matchArtifact = Artifact.builder().type("docker/.*").build() - def expectedArtifact = ExpectedArtifact.builder().id("543ef192-82a2-4805-8d0c-827f2f976a1c").matchArtifact(matchArtifact).build() + def expectedArtifact1 = ExpectedArtifact.builder().id("expected-artifact-id").matchArtifact(matchArtifact).build() + def expectedArtifact2 = ExpectedArtifact.builder().id("irrelevant-artifact-id").matchArtifact(matchArtifact).build() def receivedArtifact = Artifact.builder().name("my-artifact").type("docker/image").build() def pipeline = [ id: "abc", - trigger: [:], - expectedArtifacts: [expectedArtifact], + trigger: [ + type: "jenkins", + expectedArtifactIds: ["expected-artifact-id"] + ], + expectedArtifacts: [expectedArtifact1, expectedArtifact2], receivedArtifacts: [receivedArtifact], ] def artifactUtils = makeArtifactUtils() @@ -410,7 +414,8 @@ class ArtifactUtilsSpec extends Specification { new TypeReference>() {}) then: - resolvedArtifacts.size() == 0 + resolvedArtifacts.size() == 1 + resolvedArtifacts.get(0).getBoundArtifact() == receivedArtifact } def "resolveArtifacts adds received artifacts to the trigger, skipping duplicates"() { From 0154d4235ba343e877f1b0a224de09e26c624f62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Jervidalo?= Date: Thu, 3 Nov 2022 23:13:46 +0100 Subject: [PATCH 3/4] Add expectedArtifactIds to pipeline triggers, add more tests and fix some tests that failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jørgen Jervidalo --- .../front50/DependentPipelineStarter.groovy | 9 +- .../DependentPipelineStarterSpec.groovy | 157 +++++++++++++++++- 2 files changed, 164 insertions(+), 2 deletions(-) diff --git a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy index 954de3b1ca..ca2c1704aa 100644 --- a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy +++ b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy @@ -79,6 +79,12 @@ class DependentPipelineStarter implements ApplicationContextAware { objectMapper.writeValueAsString(pipelineConfig)) } + def expectedArtifactsIds = pipelineConfig.get("triggers", []).findAll { + it.type == "pipeline" && it.pipeline == parentPipeline.pipelineConfigId + } collectMany { + it.expectedArtifactIds ?: [] + } + pipelineConfig.trigger = [ type : "pipeline", user : authenticationDetails?.user ?: user ?: "[anonymous]", @@ -86,7 +92,8 @@ class DependentPipelineStarter implements ApplicationContextAware { parentPipelineStageId: parentPipelineStageId, parameters : [:], strategy : suppliedParameters.strategy == true, - correlationId : "${parentPipeline.id}_${parentPipelineStageId}_${pipelineConfig.id}_${parentPipeline.startTime}".toString() + correlationId : "${parentPipeline.id}_${parentPipelineStageId}_${pipelineConfig.id}_${parentPipeline.startTime}".toString(), + expectedArtifactIds : expectedArtifactsIds ] /* correlationId is added so that two pipelines aren't triggered when a pipeline is canceled. * parentPipelineStageId is added so that a child pipeline (via pipeline stage) diff --git a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarterSpec.groovy b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarterSpec.groovy index 3e8e4e7a77..aa08e08d3e 100644 --- a/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarterSpec.groovy +++ b/orca-front50/src/test/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarterSpec.groovy @@ -349,6 +349,76 @@ class DependentPipelineStarterSpec extends Specification { buildAuthenticatedUser("user", []) ) + then: + result.trigger.artifacts.size() == 2 + result.trigger.artifacts*.name.contains(testArtifact1.name) + result.trigger.artifacts*.name.contains(testArtifact2.name) + result.trigger.resolvedExpectedArtifacts.size() == 0 + } + + def "should find expected artifacts from pipeline trigger"() { + given: + def triggeredPipelineConfig = [ + name: "triggered", + id: "triggered", + expectedArtifacts: [[ + id: "id1", + matchArtifact: [ + kind: "gcs", + name: "gs://test/file.yaml", + type: "gcs/object" + ] + ]], + triggers: [[ + type: "pipeline", + pipeline: "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5", + expectedArtifactIds: ["id1"] + ], [ + type: "jenkins" + ]] + ] + Artifact testArtifact1 = Artifact.builder().type("gcs/object").name("gs://test/file.yaml").build() + Artifact testArtifact2 = Artifact.builder().type("docker/image").name("gcr.io/project/image").build() + def parentPipeline = pipeline { + name = "parent" + trigger = new DefaultTrigger("webhook", null, "test", [:], [testArtifact1, testArtifact2]) + authentication = new PipelineExecution.AuthenticationDetails("parentUser", "acct1", "acct2") + pipelineConfigId = "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5" + } + def executionLauncher = Mock(ExecutionLauncher) + def applicationContext = new StaticApplicationContext() + applicationContext.beanFactory.registerSingleton("pipelineLauncher", executionLauncher) + dependentPipelineStarter = new DependentPipelineStarter( + applicationContext, + mapper, + new ContextParameterProcessor(), + Optional.empty(), + Optional.of(artifactUtils), + new NoopRegistry() + ) + + and: + executionLauncher.start(*_) >> { _, p -> + return pipeline { + name = p.name + id = p.name + trigger = mapper.convertValue(p.trigger, Trigger) + } + } + artifactUtils.getArtifactsForPipelineId(*_) >> { + return new ArrayList(); + } + + when: + def result = dependentPipelineStarter.trigger( + triggeredPipelineConfig, + null, + parentPipeline, + [:], + null, + buildAuthenticatedUser("user", []) + ) + then: result.trigger.artifacts.size() == 2 result.trigger.artifacts*.name.contains(testArtifact1.name) @@ -357,6 +427,76 @@ class DependentPipelineStarterSpec extends Specification { result.trigger.resolvedExpectedArtifacts*.boundArtifact.name == [testArtifact1.name] } + def "should ignore expected artifacts from unrelated trigger"() { + given: + def triggeredPipelineConfig = [ + name: "triggered", + id: "triggered", + expectedArtifacts: [[ + id: "id1", + matchArtifact: [ + kind: "gcs", + name: "gs://test/file.yaml", + type: "gcs/object" + ] + ]], + triggers: [[ + type: "pipeline", + pipeline: "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5" + ], [ + type: "jenkins", + expectedArtifactIds: ["id1"] + ]] + ] + Artifact testArtifact1 = Artifact.builder().type("gcs/object").name("gs://test/file.yaml").build() + Artifact testArtifact2 = Artifact.builder().type("docker/image").name("gcr.io/project/image").build() + def parentPipeline = pipeline { + name = "parent" + trigger = new DefaultTrigger("webhook", null, "test", [:], [testArtifact1, testArtifact2]) + authentication = new PipelineExecution.AuthenticationDetails("parentUser", "acct1", "acct2") + pipelineConfigId = "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5" + } + def executionLauncher = Mock(ExecutionLauncher) + def applicationContext = new StaticApplicationContext() + applicationContext.beanFactory.registerSingleton("pipelineLauncher", executionLauncher) + dependentPipelineStarter = new DependentPipelineStarter( + applicationContext, + mapper, + new ContextParameterProcessor(), + Optional.empty(), + Optional.of(artifactUtils), + new NoopRegistry() + ) + + and: + executionLauncher.start(*_) >> { _, p -> + return pipeline { + name = p.name + id = p.name + trigger = mapper.convertValue(p.trigger, Trigger) + } + } + artifactUtils.getArtifactsForPipelineId(*_) >> { + return new ArrayList(); + } + + when: + def result = dependentPipelineStarter.trigger( + triggeredPipelineConfig, + null, + parentPipeline, + [:], + null, + buildAuthenticatedUser("user", []) + ) + + then: + result.trigger.artifacts.size() == 2 + result.trigger.artifacts*.name.contains(testArtifact1.name) + result.trigger.artifacts*.name.contains(testArtifact2.name) + result.trigger.resolvedExpectedArtifacts.size() == 0 + } + def "should resolve expressions in trigger"() { given: def triggeredPipelineConfig = [name: "triggered", id: "triggered", parameterConfig: [[name: 'a', default: '${2 == 2}']]] @@ -476,6 +616,14 @@ class DependentPipelineStarterSpec extends Specification { useDefaultArtifact: false, usePriorArtifact: true ]], + triggers: [[ + type : "pipeline", + pipeline : "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5", + expectedArtifactIds : ["28907e3a-e529-473d-bf2d-b3737c9d6dc6"] + ], [ + type : "jenkins", + expectedArtifactIds : ["unrelated_id"] + ]] ] def triggeredPipelineTemplate = mapper.convertValue([ schema: "1", @@ -506,6 +654,7 @@ class DependentPipelineStarterSpec extends Specification { name = "parent" trigger = new DefaultTrigger("manual", null, "user@schibsted.com", [:], [], [], false, true) authentication = new PipelineExecution.AuthenticationDetails("parentUser", "acct1", "acct2") + pipelineConfigId = "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5" } def executionLauncher = Mock(ExecutionLauncher) def templateLoader = Mock(TemplateLoader) @@ -580,7 +729,12 @@ class DependentPipelineStarterSpec extends Specification { ] ], schema: "1" - ] + ], + triggers: [[ + type : "pipeline", + pipeline : "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5", + expectedArtifactIds : ["helm-chart"] + ]] ] def triggeredPipelineTemplate = mapper.convertValue([ schema: "1", @@ -637,6 +791,7 @@ class DependentPipelineStarterSpec extends Specification { name = "parent" trigger = new DefaultTrigger("webhook", null, "test", [:], [testArtifact]) authentication = new PipelineExecution.AuthenticationDetails("parentUser", "acct1", "acct2") + pipelineConfigId = "5e96d1e8-a3c0-4458-b3a4-fda17e0d5ab5" } def executionLauncher = Mock(ExecutionLauncher) def templateLoader = Mock(TemplateLoader) From 1b8003f6888f66eccb18f478bc431f0f424fda0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=B8rgen=20Jervidalo?= Date: Fri, 4 Nov 2022 00:08:27 +0100 Subject: [PATCH 4/4] Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Jørgen Jervidalo --- .../spinnaker/orca/front50/DependentPipelineStarter.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy index ca2c1704aa..91e36beb8c 100644 --- a/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy +++ b/orca-front50/src/main/groovy/com/netflix/spinnaker/orca/front50/DependentPipelineStarter.groovy @@ -79,7 +79,7 @@ class DependentPipelineStarter implements ApplicationContextAware { objectMapper.writeValueAsString(pipelineConfig)) } - def expectedArtifactsIds = pipelineConfig.get("triggers", []).findAll { + def expectedArtifactIds = pipelineConfig.get("triggers", []).findAll { it.type == "pipeline" && it.pipeline == parentPipeline.pipelineConfigId } collectMany { it.expectedArtifactIds ?: [] @@ -93,7 +93,7 @@ class DependentPipelineStarter implements ApplicationContextAware { parameters : [:], strategy : suppliedParameters.strategy == true, correlationId : "${parentPipeline.id}_${parentPipelineStageId}_${pipelineConfig.id}_${parentPipeline.startTime}".toString(), - expectedArtifactIds : expectedArtifactsIds + expectedArtifactIds : expectedArtifactIds ] /* correlationId is added so that two pipelines aren't triggered when a pipeline is canceled. * parentPipelineStageId is added so that a child pipeline (via pipeline stage)