fix(jobs): fix race condition in override (#2988)

fixes a case where parameter overrides would override the base job configuration as well as the job in the context. this is because the previous implementation would assign a reference to `context[it]` instead of a copy. when the parameter overriding was done it would not only modify the fields in context but also the base configuration. this presented itself when running > 1 job in parallel. by focing a new copy (via `converValue` for simplicity) we get a fresh object reference we can assign. fixes spinnaker/spinnaker#4487
spinnaker · Jun 17, 2019 · e66bbc9 · e66bbc9
1 parent 1aaa72c
commit e66bbc9
Show file tree

Hide file tree

Showing 2 changed files with 52 additions and 3 deletions.
diff --git a/...n/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStage.groovy b/...n/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStage.groovy
@@ -16,7 +16,7 @@
 
 package com.netflix.spinnaker.orca.clouddriver.pipeline.job
 
-
+import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spinnaker.orca.clouddriver.config.PreconfiguredJobStageProperties
 import com.netflix.spinnaker.orca.clouddriver.exception.PreconfiguredJobNotFoundException
 import com.netflix.spinnaker.orca.clouddriver.service.JobService
@@ -30,11 +30,13 @@ import org.springframework.stereotype.Component
 class PreconfiguredJobStage extends RunJobStage {
 
   private JobService jobService
+  private ObjectMapper objectMapper
 
   @Autowired
   public PreconfiguredJobStage(DestroyJobTask destroyJobTask, Optional<JobService> optionalJobService) {
     super(destroyJobTask)
     this.jobService = optionalJobService.orElse(null)
+    this.objectMapper = new ObjectMapper()
   }
 
   @Override
@@ -50,9 +52,14 @@ class PreconfiguredJobStage extends RunJobStage {
   }
 
   private Map<String, Object> overrideIfNotSetInContextAndOverrideDefault(Map<String, Object> context, PreconfiguredJobStageProperties preconfiguredJob) {
+    // without converting this object, assignments to `context[it]` will result in
+    // references being assigned instead of values which causes the overrides in context
+    // to override the underlying job. this avoids that problem by giving us a fresh "copy"
+    // to work wit
+    Map<String, Object> preconfiguredMap = objectMapper.convertValue(preconfiguredJob, Map.class)
     preconfiguredJob.getOverridableFields().each {
-      if (context[it] == null || preconfiguredJob[it] != null) {
-        context[it] = preconfiguredJob[it]
+      if (context[it] == null || preconfiguredMap[it] != null) {
+        context[it] = preconfiguredMap[it]
       }
     }
     preconfiguredJob.parameters.each { defaults ->

diff --git a/...oovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStageSpec.groovy b/...oovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStageSpec.groovy
@@ -20,6 +20,7 @@ import com.netflix.spinnaker.orca.clouddriver.config.PreconfiguredJobStageParame
 import com.netflix.spinnaker.orca.clouddriver.service.JobService
 import com.netflix.spinnaker.orca.clouddriver.config.KubernetesPreconfiguredJobProperties
 import com.netflix.spinnaker.orca.clouddriver.tasks.job.DestroyJobTask
+import io.kubernetes.client.models.V1Job
 import spock.lang.Specification
 
 import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage
@@ -53,7 +54,48 @@ class PreconfiguredJobStageSpec extends Specification {
     "cloudProvider" | "kubernetes"    | "testJob" | [account: "test-account"]                                                 | new KubernetesPreconfiguredJobProperties(enabled: true, label: "testJob", type: "testJob", parameters: [], cloudProvider: "kubernetes")
     "cloudProvider" | "titus"         | "testJob" | [account: "test-account"]                                                 | new KubernetesPreconfiguredJobProperties(enabled: true, label: "testJob", type: "testJob", parameters: [new PreconfiguredJobStageParameter(mapping: "cloudProvider", defaultValue: "titus")], cloudProvider: "kubernetes")
     "cloudProvider" | "somethingElse" | "testJob" | [account: "test-account", parameters: ["cloudProvider": "somethingElse"]] | new KubernetesPreconfiguredJobProperties(enabled: true, label: "testJob", type: "testJob", parameters: [new PreconfiguredJobStageParameter(mapping: "cloudProvider", defaultValue: "titus", "name": "cloudProvider")], cloudProvider: "kubernetes")
+  }
+
+  def "should use copy of preconfigured job to populate context"() {
+
+    given:
+    def manifestMetadataName = "defaultName"
+    def overriddenName = "fromParameter"
+    def stage = stage {
+      type = "test"
+      context = [account: "test"]
+    }
+    def property = new KubernetesPreconfiguredJobProperties(
+      enabled: true,
+      label: "test",
+      type: "test",
+      parameters: [
+        new PreconfiguredJobStageParameter(
+          mapping: "manifest.metadata.name",
+          defaultValue: "fromParameter",
+          name: "metadataName"
+        )
+      ],
+      manifest: new V1Job(metadata: [name: "defaultName"])
+    )
+
+    def jobService = Mock(JobService) {
+      2 * getPreconfiguredStages() >> {
+        return [
+          property
+        ]
+      }
+    }
 
+    when:
+    PreconfiguredJobStage preconfiguredJobStage = new PreconfiguredJobStage(Mock(DestroyJobTask), Optional.of(jobService))
+    preconfiguredJobStage.buildTaskGraph(stage)
 
+    then:
+    // verify that underlying job configuration hasn't been modified
+    def preconfiguredJob = (KubernetesPreconfiguredJobProperties) jobService.getPreconfiguredStages().get(0)
+    preconfiguredJob.getManifest().getMetadata().getName() == manifestMetadataName
+    // verify that stage manifest has the correctly overridden name
+    stage.getContext().get("manifest").metadata.name == overriddenName
   }
 }