Skip to content

Commit

Permalink
Merge pull request #537 from Vlatombe/JENKINS-58374
Browse files Browse the repository at this point in the history 
[JENKINS-58374] Properly merge environment variable lists to avoid duplicates
  • Loading branch information
@Vlatombe
Vlatombe authored Jul 8, 2019
2 parents 485de42 + 9448a32 commit 113be70
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 23 deletions.
50 changes: 37 additions & 13 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,14 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.BinaryOperator;
import java.util.function.Function;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand All @@ -29,6 +31,7 @@
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.lang.StringUtils;
import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.volumes.PodVolume;
Expand Down Expand Up @@ -575,24 +578,45 @@ public static boolean validateLabel(String label) {
}

private static List<EnvVar> combineEnvVars(Container parent, Container template) {
List<EnvVar> combinedEnvVars = new ArrayList<>();
combinedEnvVars.addAll(parent.getEnv());
combinedEnvVars.addAll(template.getEnv());
return combinedEnvVars.stream().filter(envVar -> !Strings.isNullOrEmpty(envVar.getName())).collect(toList());
Map<String,EnvVar> combinedEnvVars = mergeMaps(envVarstoMap(parent.getEnv()),envVarstoMap(template.getEnv()));
return combinedEnvVars.entrySet().stream()
.filter(envVar -> !Strings.isNullOrEmpty(envVar.getKey()))
.map(Map.Entry::getValue)
.collect(toList());
}

@VisibleForTesting
static Map<String, EnvVar> envVarstoMap(List<EnvVar> envVarList) {
return envVarList.stream().collect(toMap(EnvVar::getName, Function.identity()));
}

private static List<TemplateEnvVar> combineEnvVars(ContainerTemplate parent, ContainerTemplate template) {
List<TemplateEnvVar> combinedEnvVars = new ArrayList<>();
combinedEnvVars.addAll(parent.getEnvVars());
combinedEnvVars.addAll(template.getEnvVars());
return combinedEnvVars.stream().filter(envVar -> !Strings.isNullOrEmpty(envVar.getKey())).collect(toList());
return combineEnvVars(parent.getEnvVars(), template.getEnvVars());
}

private static List<TemplateEnvVar> combineEnvVars(PodTemplate parent, PodTemplate template) {
List<TemplateEnvVar> combinedEnvVars = new ArrayList<>();
combinedEnvVars.addAll(parent.getEnvVars());
combinedEnvVars.addAll(template.getEnvVars());
return combinedEnvVars.stream().filter(envVar -> !Strings.isNullOrEmpty(envVar.getKey())).collect(toList());
return combineEnvVars(parent.getEnvVars(), template.getEnvVars());
}

private static List<TemplateEnvVar> combineEnvVars(List<TemplateEnvVar> parent, List<TemplateEnvVar> child) {
Map<String,TemplateEnvVar> combinedEnvVars = mergeMaps(templateEnvVarstoMap(parent),templateEnvVarstoMap(child));
return combinedEnvVars
.entrySet()
.stream()
.filter(entry -> !Strings.isNullOrEmpty(entry.getKey()))
.map(Map.Entry::getValue)
.collect(toList());
}

@VisibleForTesting
static Map<String, TemplateEnvVar> templateEnvVarstoMap(List<TemplateEnvVar> envVarList) {
return envVarList
.stream()
.collect(Collectors.toMap(TemplateEnvVar::getKey, Function.identity(), throwingMerger(), LinkedHashMap::new));
}

private static <T> BinaryOperator<T> throwingMerger() {
return (u,v) -> { throw new IllegalStateException(String.format("Duplicate key %s", u)); };
}

private static List<EnvFromSource> combinedEnvFromSources(Container parent, Container template) {
Expand All @@ -606,7 +630,7 @@ private static List<EnvFromSource> combinedEnvFromSources(Container parent, Cont
}

private static <K, V> Map<K, V> mergeMaps(Map<K, V> m1, Map<K, V> m2) {
Map<K, V> m = new HashMap<>();
Map<K, V> m = new LinkedHashMap<>();
if (m1 != null)
m.putAll(m1);
if (m2 != null)
Expand Down
52 changes: 42 additions & 10 deletions src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateBuilderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,8 +109,7 @@ public void testBuildFromYaml() throws Exception {
validatePod(pod);
assertThat(pod.getMetadata().getLabels(), hasEntry("jenkins", "slave"));

Map<String, Container> containers = pod.getSpec().getContainers().stream()
.collect(Collectors.toMap(Container::getName, Function.identity()));
Map<String, Container> containers = toContainerMap(pod);
assertEquals(2, containers.size());

Container container0 = containers.get("busybox");
Expand Down Expand Up @@ -185,8 +184,7 @@ private void validatePod(Pod pod, boolean fromYaml) {
assertThat(pod.getMetadata().getLabels(), hasEntry("some-label", "some-label-value"));

// check containers
Map<String, Container> containers = pod.getSpec().getContainers().stream()
.collect(Collectors.toMap(Container::getName, Function.identity()));
Map<String, Container> containers = toContainerMap(pod);
assertEquals(2, containers.size());

assertEquals("busybox", containers.get("busybox").getImage());
Expand Down Expand Up @@ -261,8 +259,7 @@ public void testOverridesFromYaml() throws Exception {
setupStubs();
Pod pod = new PodTemplateBuilder(template).withSlave(slave).build();

Map<String, Container> containers = pod.getSpec().getContainers().stream()
.collect(Collectors.toMap(Container::getName, Function.identity()));
Map<String, Container> containers = toContainerMap(pod);
assertEquals(1, containers.size());
Container jnlp = containers.get("jnlp");
assertEquals("Wrong number of volume mounts: " + jnlp.getVolumeMounts(), 1, jnlp.getVolumeMounts().size());
Expand Down Expand Up @@ -296,8 +293,7 @@ public void testInheritsFromWithYaml() throws Exception {
PodTemplate result = combine(parent, template);
Pod pod = new PodTemplateBuilder(result).withSlave(slave).build();

Map<String, Container> containers = pod.getSpec().getContainers().stream()
.collect(Collectors.toMap(Container::getName, Function.identity()));
Map<String, Container> containers = toContainerMap(pod);
assertEquals(1, containers.size());
Container jnlp = containers.get("jnlp");
assertEquals(new Quantity("1"), jnlp.getResources().getLimits().get("cpu"));
Expand Down Expand Up @@ -386,6 +382,38 @@ public void yamlOverrideContainer() throws Exception {
assertEquals("busybox2", container.get().getImage());
}

@Issue("JENKINS-58374")
@Test
public void yamlOverrideContainerEnvvar() throws Exception {
PodTemplate parent = new PodTemplate();
parent.setYaml("kind: Pod\n" +
"spec:\n" +
" containers:\n" +
" - name: jnlp\n" +
" env:\n" +
" - name: VAR1\n" +
" value: \"1\"\n" +
" - name: VAR2\n" +
" value: \"1\"\n");
PodTemplate child = new PodTemplate();
child.setYaml("kind: Pod\n" +
"spec:\n" +
" containers:\n" +
" - name: jnlp\n" +
" env:\n" +
" - name: VAR1\n" +
" value: \"2\"\n");
setupStubs();

PodTemplate result = combine(parent, child);
Pod pod = new PodTemplateBuilder(result).withSlave(slave).build();
Map<String, Container> containers = toContainerMap(pod);
Container jnlp = containers.get("jnlp");
Map<String, EnvVar> env = PodTemplateUtils.envVarstoMap(jnlp.getEnv());
assertEquals("2", env.get("VAR1").getValue()); // value from child
assertEquals("1", env.get("VAR2").getValue()); // value from parent
}

@Test
public void yamlOverrideVolume() throws Exception {
PodTemplate parent = new PodTemplate();
Expand Down Expand Up @@ -468,14 +496,18 @@ public void testOverridesContainerSpec() throws Exception {
setupStubs();
Pod pod = new PodTemplateBuilder(template).withSlave(slave).build();

Map<String, Container> containers = pod.getSpec().getContainers().stream()
.collect(Collectors.toMap(Container::getName, Function.identity()));
Map<String, Container> containers = toContainerMap(pod);
assertEquals(1, containers.size());
Container jnlp = containers.get("jnlp");
assertEquals("Wrong number of volume mounts: " + jnlp.getVolumeMounts(), 1, jnlp.getVolumeMounts().size());
validateJnlpContainer(jnlp, slave);
}

private Map<String, Container> toContainerMap(Pod pod) {
return pod.getSpec().getContainers().stream()
.collect(Collectors.toMap(Container::getName, Function.identity()));
}

private String loadYamlFile(String s) throws IOException {
return new String(IOUtils.toByteArray(getClass().getResourceAsStream(s)));
}
Expand Down

0 comments on commit 113be70

Please sign in to comment.