From e07fa946010a317f85df712de88b3341cb5b5ba6 Mon Sep 17 00:00:00 2001 From: Joe DeStefano Date: Thu, 6 Aug 2020 13:05:11 -0700 Subject: [PATCH] fix(preconfigured): Allow PreconfiguredJobStage parameters to map to one-dimensional array property values (#3830) * fix(preconfigured): Update setNestedValue to handle one-dimensional array objects * fix(preconfigured): Handle case where array property is missing from root object * refactor(preconfigured): Use Groovy regex syntax * refactor(preconfigured): Update exception message to be more meaningful Co-authored-by: Mark Vulfson Co-authored-by: Mark Vulfson (cherry picked from commit 3fa29ca79172024c96b316ec2173272788136bdd) --- .../pipeline/job/PreconfiguredJobStage.groovy | 33 ++++- .../job/PreconfiguredJobStageSpec.groovy | 125 ++++++++++++++++++ 2 files changed, 155 insertions(+), 3 deletions(-) diff --git a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStage.groovy b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStage.groovy index c5ef2be502..88af59d8c0 100644 --- a/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStage.groovy +++ b/orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStage.groovy @@ -28,6 +28,8 @@ import org.springframework.stereotype.Component import javax.annotation.Nonnull +import java.util.List + @Component class PreconfiguredJobStage extends RunJobStage { @@ -95,13 +97,38 @@ class PreconfiguredJobStage extends RunJobStage { String[] props = mapping.split(/\./) Object current = root for (int i = 0; i < props.length - 1; i++) { - Object next = current[props[i]] - if (next == null) { - throw new IllegalArgumentException("no property ${props[i]} on $current") + Object next + if(props[i] ==~ /.*\[\d+\]$/) { + next = getValueFromArrayExpression(current, props[i]) + } else { + next = current[props[i]] + if (next == null) { + throw new IllegalArgumentException("no property ${props[i]} on $current") + } } current = next } current[props.last()] = value } + private static Object getValueFromArrayExpression(Object root, String expression) { + // TODO: Do we need to handle arrays nested in other arrays? + String[] parts = expression.split(/[\[\]]/) + String propName = parts[0] + int index = -1 + try { + index = Integer.parseInt(parts[1]) + } catch (NumberFormatException ex) { + throw new IllegalArgumentException("Unable to parse index from expresion ${expression}", ex) + } + List nextList = root[propName] + if (nextList == null) { + throw new IllegalArgumentException("no property ${propName} on $root") + } + if (nextList.size() <= index || index < 0) { + throw new IllegalArgumentException("Invalid index $index for list $propName") + } + return nextList.get(index) + } + } diff --git a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStageSpec.groovy b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStageSpec.groovy index d91df55df0..1c9278074c 100644 --- a/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStageSpec.groovy +++ b/orca-clouddriver/src/test/groovy/com/netflix/spinnaker/orca/clouddriver/pipeline/job/PreconfiguredJobStageSpec.groovy @@ -20,6 +20,8 @@ import com.netflix.spinnaker.orca.api.preconfigured.jobs.PreconfiguredJobStagePa 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.V1Container +import io.kubernetes.client.models.V1EnvVar import io.kubernetes.client.models.V1Job import spock.lang.Specification @@ -99,4 +101,127 @@ class PreconfiguredJobStageSpec extends Specification { // verify that stage manifest has the correctly overridden name stage.getContext().get("manifest").metadata.name == overriddenName } + + def "setNestedValue can handle array property mappings"() { + given: + def manifestEnvValue = "defaultValue" + def overriddenValue = "newValue" + def stage = stage { + type = "test" + context = [account: "test"] + } + def property = new KubernetesPreconfiguredJobProperties( + enabled: true, + label: "test", + type: "test", + cloudProvider: "kubernetes", + parameters: [ + new PreconfiguredJobStageParameter( + mapping: "manifest.spec.template.spec.containers[0].env[0].value", + defaultValue: overriddenValue, + name: "envVariable" + ) + ], + manifest: new V1Job(spec: [template: [spec: [containers: [new V1Container(env: [new V1EnvVar(name: "foo", value: manifestEnvValue)])]]]]) + ) + + 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().getSpec().getTemplate().getSpec().getContainers()[0].getEnv()[0].getValue() == manifestEnvValue + // verify that stage manifest has the correctly overridden name + stage.getContext().get("manifest").spec.template.spec.containers[0].env[0].value == overriddenValue + } + + def "setNestedValue throws an error if the index is invalid"() { + given: + def manifestEnvValue = "defaultValue" + def overriddenValue = "newValue" + def stage = stage { + type = "test" + context = [account: "test"] + } + def property = new KubernetesPreconfiguredJobProperties( + enabled: true, + label: "test", + type: "test", + cloudProvider: "kubernetes", + parameters: [ + new PreconfiguredJobStageParameter( + mapping: "manifest.spec.template.spec.containers[0].env[1].value", + defaultValue: overriddenValue, + name: "envVariable" + ) + ], + manifest: new V1Job(spec: [template: [spec: [containers: [new V1Container(env: [new V1EnvVar(name: "foo", value: manifestEnvValue)])]]]]) + ) + + def jobService = Mock(JobService) { + 1 * getPreconfiguredStages() >> { + return [ + property + ] + } + } + + when: + PreconfiguredJobStage preconfiguredJobStage = new PreconfiguredJobStage(Mock(DestroyJobTask), [], Optional.of(jobService)) + preconfiguredJobStage.buildTaskGraph(stage) + + then: + def ex = thrown(IllegalArgumentException) + assert ex.getMessage().startsWith("Invalid index 1 for list") + } + + def "setNestedValue throws an error if an array property name is invalid"() { + given: + def manifestEnvValue = "defaultValue" + def overriddenValue = "newValue" + def stage = stage { + type = "test" + context = [account: "test"] + } + def property = new KubernetesPreconfiguredJobProperties( + enabled: true, + label: "test", + type: "test", + cloudProvider: "kubernetes", + parameters: [ + new PreconfiguredJobStageParameter( + mapping: "manifest.spec.template.spec.containers[0].missingProperty[0].value", + defaultValue: overriddenValue, + name: "envVariable" + ) + ], + manifest: new V1Job(spec: [template: [spec: [containers: [new V1Container(env: [new V1EnvVar(name: "foo", value: manifestEnvValue)])]]]]) + ) + + def jobService = Mock(JobService) { + 1 * getPreconfiguredStages() >> { + return [ + property + ] + } + } + + when: + PreconfiguredJobStage preconfiguredJobStage = new PreconfiguredJobStage(Mock(DestroyJobTask), [], Optional.of(jobService)) + preconfiguredJobStage.buildTaskGraph(stage) + + then: + def ex = thrown(IllegalArgumentException) + assert ex.getMessage().startsWith("no property missingProperty on") + } }