[JENKINS-58405] Backward compatibility for yaml field merge (#542)

[JENKINS-58405] Backward compatibility for yaml field merge
jenkinsci · Jul 10, 2019 · 6a244cf · 6a244cf
2 parents 839bcc2 + 159d40e
commit 6a244cf
Show file tree

Hide file tree

Showing 17 changed files with 212 additions and 3 deletions.
diff --git a/README.md b/README.md
@@ -137,6 +137,7 @@ Either way it provides access to the following fields:
 * **namespace** The namespace of the pod.
 * **label** The label of the pod. Can be set to a unique value to avoid conflicts across builds, or omitted and `POD_LABEL` will be defined inside the step.
 * **yaml** [yaml representation of the Pod](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.10/#pod-v1-core), to allow setting any values not supported as fields
+* **yamlMergeStrategy** `merge()` or `override()`. Controls whether the yaml definition overrides or is merged with the yaml definition inherited from pod templates declared with `inheritFrom`. Defaults to `override()`.
 * **containers** The container templates that are use to create the containers of the pod *(see below)*.
 * **serviceAccount** The service account of the pod.
 * **nodeSelector** The node selector of the pod.

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
@@ -16,6 +16,7 @@
 import org.apache.commons.lang.StringUtils;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.pod.retention.PodRetention;
+import org.csanchez.jenkins.plugins.kubernetes.pod.yaml.YamlMergeStrategy;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.workspace.WorkspaceVolume;
 import org.kohsuke.accmod.Restricted;
@@ -129,6 +130,21 @@ public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements
 
     private List<String> yamls = new ArrayList<>();
 
+    public YamlMergeStrategy getYamlMergeStrategy() {
+        return yamlMergeStrategy;
+    }
+
+    @DataBoundSetter
+    public void setYamlMergeStrategy(YamlMergeStrategy yamlMergeStrategy) {
+        this.yamlMergeStrategy = yamlMergeStrategy;
+    }
+
+    private YamlMergeStrategy yamlMergeStrategy = YamlMergeStrategy.defaultStrategy();
+
+    public Pod getYamlsPod() {
+        return yamlMergeStrategy.merge(getYamls());
+    }
+
     private Boolean showRawYaml;
 
     @CheckForNull
@@ -718,6 +734,10 @@ protected Object readResolve() {
             showRawYaml = Boolean.TRUE;
         }
 
+        if (yamlMergeStrategy == null) {
+            yamlMergeStrategy = YamlMergeStrategy.defaultStrategy();
+        }
+
         return this;
     }
 
@@ -805,7 +825,8 @@ public List<? extends Descriptor> getEnvVarsDescriptors() {
             return DescriptorVisibilityFilter.apply(null, Jenkins.getInstance().getDescriptorList(TemplateEnvVar.class));
         }
 
-        @SuppressWarnings("rawtypes")
+        @SuppressWarnings("unused") // Used by jelly
+        @Restricted(DoNotUse.class) // Used by jelly
         public Descriptor getDefaultPodRetention() {
             Jenkins jenkins = Jenkins.getInstanceOrNull();
             if (jenkins == null) {
@@ -814,6 +835,11 @@ public Descriptor getDefaultPodRetention() {
             return jenkins.getDescriptor(PodRetention.getPodTemplateDefault().getClass());
         }
 
+        @SuppressWarnings("unused") // Used by jelly
+        @Restricted(DoNotUse.class) // Used by jelly
+        public YamlMergeStrategy getDefaultYamlMergeStrategy() {
+            return YamlMergeStrategy.defaultStrategy();
+        }
     }
 
     @Override

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilder.java
@@ -198,8 +198,7 @@ public Pod build() {
         builder.withContainers(containers.values().toArray(new Container[containers.size()]));
 
         // merge with the yaml fragments
-        Pod yamlPods = combine(template.getYamls().stream().map(PodTemplateUtils::parseFromYaml).collect(Collectors.toList()));
-        Pod pod = combine(yamlPods, builder.endSpec().build());
+        Pod pod = combine(template.getYamlsPod(), builder.endSpec().build());
 
         // Apply defaults
 

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java
@@ -351,6 +351,7 @@ public static PodTemplate combine(PodTemplate parent, PodTemplate template) {
         podTemplate.setAnnotations(new ArrayList<>(podAnnotations));
         podTemplate.setNodeProperties(toolLocationNodeProperties);
         podTemplate.setNodeUsageMode(nodeUsageMode);
+        podTemplate.setYamlMergeStrategy(template.getYamlMergeStrategy());
         podTemplate.setInheritFrom(!Strings.isNullOrEmpty(template.getInheritFrom()) ?
                                    template.getInheritFrom() : parent.getInheritFrom());
 

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStep.java
@@ -13,6 +13,7 @@
 import org.csanchez.jenkins.plugins.kubernetes.PodTemplate;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.pod.retention.PodRetention;
+import org.csanchez.jenkins.plugins.kubernetes.pod.yaml.YamlMergeStrategy;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.workspace.WorkspaceVolume;
 import org.jenkinsci.plugins.workflow.steps.Step;
@@ -60,6 +61,7 @@ public class PodTemplateStep extends Step implements Serializable {
     private String workingDir = ContainerTemplate.DEFAULT_WORKING_DIR;
 
     private String yaml;
+    private YamlMergeStrategy yamlMergeStrategy = YamlMergeStrategy.defaultStrategy();
     private PodRetention podRetention;
 
     private Boolean showRawYaml;
@@ -133,6 +135,15 @@ public void setEnvVars(List<TemplateEnvVar> envVars) {
         }
     }
 
+    public YamlMergeStrategy getYamlMergeStrategy() {
+        return yamlMergeStrategy;
+    }
+
+    @DataBoundSetter
+    public void setYamlMergeStrategy(YamlMergeStrategy yamlMergeStrategy) {
+        this.yamlMergeStrategy = yamlMergeStrategy;
+    }
+
     public List<PodVolume> getVolumes() {
         return volumes;
     }

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/PodTemplateStepExecution.java
@@ -113,6 +113,7 @@ public boolean start() throws Exception {
         newTemplate.setNodeUsageMode(step.getNodeUsageMode());
         newTemplate.setServiceAccount(step.getServiceAccount());
         newTemplate.setAnnotations(step.getAnnotations());
+        newTemplate.setYamlMergeStrategy(step.getYamlMergeStrategy());
         if(run!=null) {
             newTemplate.getAnnotations().add(new PodAnnotation("buildUrl", ((KubernetesCloud)cloud).getJenkinsUrlOrDie()+run.getUrl()));
         }

diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Merge.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Merge.java
@@ -0,0 +1,37 @@
+package org.csanchez.jenkins.plugins.kubernetes.pod.yaml;
+
+import hudson.Extension;
+import hudson.model.Descriptor;
+import io.fabric8.kubernetes.api.model.Pod;
+import org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils;
+import org.jenkinsci.Symbol;
+import org.kohsuke.stapler.DataBoundConstructor;
+
+import javax.annotation.Nonnull;
+import java.util.List;
+import java.util.stream.Collectors;
+
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.combine;
+
+public class Merge extends YamlMergeStrategy {
+    private static final long serialVersionUID = 6562610892063268131L;
+
+    @DataBoundConstructor
+    public Merge() {
+    }
+
+    @Override
+    public Pod merge(List<String> yamls) {
+        return combine(yamls.stream().map(PodTemplateUtils::parseFromYaml).collect(Collectors.toList()));
+    }
+
+    @Extension
+    @Symbol("merge")
+    public static class DescriptorImpl extends Descriptor<YamlMergeStrategy> {
+        @Nonnull
+        @Override
+        public String getDisplayName() {
+            return "Merge";
+        }
+    }
+}
diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Overrides.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Overrides.java
@@ -0,0 +1,39 @@
+package org.csanchez.jenkins.plugins.kubernetes.pod.yaml;
+
+import hudson.Extension;
+import hudson.model.Descriptor;
+import io.fabric8.kubernetes.api.model.Pod;
+import org.jenkinsci.Symbol;
+import org.kohsuke.stapler.DataBoundConstructor;
+
+import javax.annotation.Nonnull;
+import java.util.List;
+
+import static org.csanchez.jenkins.plugins.kubernetes.PodTemplateUtils.parseFromYaml;
+
+public class Overrides extends YamlMergeStrategy {
+    private static final long serialVersionUID = 4510341864619338164L;
+
+    @DataBoundConstructor
+    public Overrides() {
+    }
+
+    @Override
+    public Pod merge(List<String> yamls) {
+        if (yamls.isEmpty()) {
+            return null;
+        } else {
+            return parseFromYaml(yamls.get(yamls.size()-1));
+        }
+    }
+
+    @Extension
+    @Symbol("override")
+    public static class DescriptorImpl extends Descriptor<YamlMergeStrategy> {
+        @Nonnull
+        @Override
+        public String getDisplayName() {
+            return "Override";
+        }
+    }
+}
diff --git a/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/YamlMergeStrategy.java b/src/main/java/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/YamlMergeStrategy.java
@@ -0,0 +1,17 @@
+package org.csanchez.jenkins.plugins.kubernetes.pod.yaml;
+
+import hudson.ExtensionPoint;
+import hudson.model.AbstractDescribableImpl;
+import io.fabric8.kubernetes.api.model.Pod;
+
+import javax.annotation.Nonnull;
+import java.io.Serializable;
+import java.util.List;
+
+public abstract class YamlMergeStrategy extends AbstractDescribableImpl<YamlMergeStrategy> implements ExtensionPoint, Serializable {
+    public static YamlMergeStrategy defaultStrategy() {
+        return new Overrides();
+    }
+
+    public abstract Pod merge(@Nonnull List<String> yamls);
+}
diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/config.jelly b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/config.jelly
@@ -71,6 +71,8 @@
     <f:textarea/>
   </f:entry>
 
+  <f:dropdownDescriptorSelector title="${%Yaml merge strategy}" field="yamlMergeStrategy" default="${descriptor.defaultYamlMergeStrategy}" />
+
   <f:entry field="showRawYaml" title="${%Show raw yaml in console}" >
     <f:checkbox default="true"/>
   </f:entry>

diff --git a/...resources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/help-yamlMergeStrategy.html b/...resources/org/csanchez/jenkins/plugins/kubernetes/PodTemplate/help-yamlMergeStrategy.html
@@ -0,0 +1 @@
+Defines how the raw yaml field gets merged with yaml definitions from inherited pod templates.
diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Merge/config.jelly b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Merge/config.jelly
@@ -0,0 +1,5 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
+         xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:description>The yaml definitions from inherited pod templates are merged with the current template yaml fragment.</f:description>
+</j:jelly>
diff --git a/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Overrides/config.jelly b/src/main/resources/org/csanchez/jenkins/plugins/kubernetes/pod/yaml/Overrides/config.jelly
@@ -0,0 +1,5 @@
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout"
+         xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:description>The yaml definitions from inherited pod templates are completely overridden by the current yaml field.</f:description>
+</j:jelly>
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilderTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilderTest.java
@@ -20,6 +20,8 @@
 import org.apache.commons.compress.utils.IOUtils;
 import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
 import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
+import org.csanchez.jenkins.plugins.kubernetes.pod.yaml.Merge;
+import org.csanchez.jenkins.plugins.kubernetes.pod.yaml.YamlMergeStrategy;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume;
 import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
@@ -344,6 +346,7 @@ public void yamlMergeContainers() throws Exception {
                 "    - cat\n" +
                 "    tty: true\n"
         );
+        child.setYamlMergeStrategy(merge());
         child.setInheritFrom("parent");
         setupStubs();
         PodTemplate result = combine(parent, child);
@@ -385,6 +388,7 @@ public void yamlOverrideContainer() throws Exception {
                 "    tty: true\n"
         );
         child.setInheritFrom("parent");
+        child.setYamlMergeStrategy(merge());
         setupStubs();
         PodTemplate result = combine(parent, child);
         Pod pod = new PodTemplateBuilder(result).withSlave(slave).build();
@@ -409,6 +413,7 @@ public void yamlOverrideContainerEnvvar() throws Exception {
                 "    - name: VAR2\n" +
                 "      value: \"1\"\n");
         PodTemplate child = new PodTemplate();
+        child.setYamlMergeStrategy(merge());
         child.setYaml("kind: Pod\n" +
                 "spec:\n" +
                 "  containers:\n" +
@@ -452,6 +457,7 @@ public void yamlOverrideVolume() throws Exception {
                 "      path: /host/data2\n"
         );
         child.setInheritFrom("parent");
+        child.setYamlMergeStrategy(merge());
         setupStubs();
         PodTemplate result = combine(parent, child);
         Pod pod = new PodTemplateBuilder(result).withSlave(slave).build();
@@ -487,6 +493,7 @@ public void yamlMergeVolumes() throws Exception {
                 "      path: /host/data2\n"
         );
         child.setInheritFrom("parent");
+        child.setYamlMergeStrategy(merge());
         setupStubs();
         PodTemplate result = combine(parent, child);
         Pod pod = new PodTemplateBuilder(result).withSlave(slave).build();
@@ -524,4 +531,8 @@ private Map<String, Container> toContainerMap(Pod pod) {
     private String loadYamlFile(String s) throws IOException {
         return new String(IOUtils.toByteArray(getClass().getResourceAsStream(s)));
     }
+
+    private YamlMergeStrategy merge() {
+        return new Merge();
+    }
 }
diff --git a/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java b/src/test/java/org/csanchez/jenkins/plugins/kubernetes/pipeline/KubernetesPipelineTest.java
@@ -392,4 +392,16 @@ public void runInPodWithShowRawYamlFalse() throws Exception {
         r.assertBuildStatusSuccess(r.waitForCompletion(b));
         r.assertLogNotContains("value: container-env-var-value", b);
     }
+
+    @Test
+    @Issue("JENKINS-58405")
+    public void overrideYaml() throws Exception {
+        r.assertBuildStatusSuccess(r.waitForCompletion(b));
+    }
+
+    @Test
+    @Issue("JENKINS-58405")
+    public void mergeYaml() throws Exception {
+        r.assertBuildStatusSuccess(r.waitForCompletion(b));
+    }
 }
diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/mergeYaml.groovy b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/mergeYaml.groovy
@@ -0,0 +1,21 @@
+podTemplate(yaml: """
+spec:
+    containers:
+    - name: jnlp
+    env:
+    - name: VAR1
+      value: 1
+""") {
+    podTemplate(yamlMergeStrategy: merge(),
+                yaml: """
+    containers:
+    - name: jnlp
+    env:
+    - name: VAR2
+      value: 1
+""") {
+        node(POD_LABEL){
+            sh '["$VAR1" == "$VAR2"]'
+        }
+    }
+}
diff --git a/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/overrideYaml.groovy b/src/test/resources/org/csanchez/jenkins/plugins/kubernetes/pipeline/overrideYaml.groovy
@@ -0,0 +1,20 @@
+podTemplate(yaml: """
+spec:
+    containers:
+    - name: jnlp
+    env:
+    - name: VAR1
+      value: 1
+""") {
+    podTemplate(yaml: """
+    containers:
+    - name: jnlp
+    env:
+    - name: VAR2
+      value: 1
+""") {
+        node(POD_LABEL){
+            sh '["$VAR1" != "$VAR2"]'
+        }
+    }
+}
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		Defines how the raw yaml field gets merged with yaml definitions from inherited pod templates.