Skip to content

Commit 1f1b028

Browse files
amarkdotdevamarkdotdev
committed
Merge supplementalGroups when combining pod YAML with builder spec
PodTemplateUtils.combine() only copied runAsUser and runAsGroup into the merged pod securityContext. supplementalGroups configured on the pod template (UI, CasC, or pipeline) were dropped whenever raw pod YAML was present with the Override merge strategy. Add supplementalGroups resolution alongside the existing pod-level security context merge and cover the regression in unit tests. Signed-off-by: amarkdotdev <amarkdotdev@users.noreply.github.com>
1 parent 62200f8 commit 1f1b028

3 files changed

Lines changed: 162 additions & 32 deletions

File tree

‎src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtils.java‎

Lines changed: 36 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import io.fabric8.kubernetes.api.model.ObjectMeta;
2323
import io.fabric8.kubernetes.api.model.Pod;
2424
import io.fabric8.kubernetes.api.model.PodBuilder;
25+
import io.fabric8.kubernetes.api.model.PodSecurityContext;
2526
import io.fabric8.kubernetes.api.model.PodSpec;
2627
import io.fabric8.kubernetes.api.model.Quantity;
2728
import io.fabric8.kubernetes.api.model.ResourceRequirements;
@@ -209,6 +210,30 @@ private static Map<String, Quantity> combineResources(
209210
);
210211
}
211212

213+
private static Long resolvePodSecurityContextLong(
214+
PodSecurityContext parent,
215+
PodSecurityContext template,
216+
Function<PodSecurityContext, Long> getter) {
217+
Long templateValue = template != null ? getter.apply(template) : null;
218+
if (templateValue != null) {
219+
return templateValue;
220+
}
221+
return parent != null ? getter.apply(parent) : null;
222+
}
223+
224+
private static List<Long> resolvePodSecurityContextSupplementalGroups(
225+
PodSecurityContext parent, PodSecurityContext template) {
226+
List<Long> templateValue = template != null ? template.getSupplementalGroups() : null;
227+
if (templateValue != null && !templateValue.isEmpty()) {
228+
return templateValue;
229+
}
230+
List<Long> parentValue = parent != null ? parent.getSupplementalGroups() : null;
231+
if (parentValue != null && !parentValue.isEmpty()) {
232+
return parentValue;
233+
}
234+
return null;
235+
}
236+
212237
private static Capabilities combineCapabilities(Container parent, Container template) {
213238
Capabilities parentCapabilities = parent.getSecurityContext() != null
214239
? parent.getSecurityContext().getCapabilities()
@@ -343,40 +368,19 @@ public static Pod combine(Pod parent, Pod template) {
343368

344369
// Security context
345370
if (template.getSpec().getSecurityContext() != null || parent.getSpec().getSecurityContext() != null) {
371+
PodSecurityContext parentSecurityContext = parent.getSpec().getSecurityContext();
372+
PodSecurityContext templateSecurityContext = template.getSpec().getSecurityContext();
373+
Long runAsUser = resolvePodSecurityContextLong(
374+
parentSecurityContext, templateSecurityContext, PodSecurityContext::getRunAsUser);
375+
Long runAsGroup = resolvePodSecurityContextLong(
376+
parentSecurityContext, templateSecurityContext, PodSecurityContext::getRunAsGroup);
377+
List<Long> supplementalGroups =
378+
resolvePodSecurityContextSupplementalGroups(parentSecurityContext, templateSecurityContext);
346379
specBuilder
347380
.editOrNewSecurityContext()
348-
.withRunAsUser(
349-
template.getSpec().getSecurityContext() != null
350-
&& template.getSpec()
351-
.getSecurityContext()
352-
.getRunAsUser()
353-
!= null
354-
? template.getSpec().getSecurityContext().getRunAsUser()
355-
: (parent.getSpec().getSecurityContext() != null
356-
&& parent.getSpec()
357-
.getSecurityContext()
358-
.getRunAsUser()
359-
!= null
360-
? parent.getSpec()
361-
.getSecurityContext()
362-
.getRunAsUser()
363-
: null))
364-
.withRunAsGroup(
365-
template.getSpec().getSecurityContext() != null
366-
&& template.getSpec()
367-
.getSecurityContext()
368-
.getRunAsGroup()
369-
!= null
370-
? template.getSpec().getSecurityContext().getRunAsGroup()
371-
: (parent.getSpec().getSecurityContext() != null
372-
&& parent.getSpec()
373-
.getSecurityContext()
374-
.getRunAsGroup()
375-
!= null
376-
? parent.getSpec()
377-
.getSecurityContext()
378-
.getRunAsGroup()
379-
: null))
381+
.withRunAsUser(runAsUser)
382+
.withRunAsGroup(runAsGroup)
383+
.withSupplementalGroups(supplementalGroups)
380384
.endSecurityContext();
381385
}
382386

‎src/test/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplateUtilsTest.java‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,33 @@ public void shouldCombineActiveDeadlineSeconds() {
468468
assertEquals(combinedPod.getSpec().getActiveDeadlineSeconds(), Long.valueOf(1L));
469469
}
470470

471+
@Test
472+
public void shouldCombinePodSupplementalGroupsFromTemplate() {
473+
Pod parentPod = new PodBuilder()
474+
.withNewMetadata()
475+
.endMetadata()
476+
.withNewSpec()
477+
.withContainers(new ContainerBuilder().withName("jnlp").build())
478+
.endSpec()
479+
.build();
480+
Pod childPod = new PodBuilder()
481+
.withNewMetadata()
482+
.endMetadata()
483+
.withNewSpec()
484+
.withNewSecurityContext()
485+
.withRunAsUser(1000L)
486+
.withRunAsGroup(1000L)
487+
.withSupplementalGroups(5001L, 5002L)
488+
.endSecurityContext()
489+
.endSpec()
490+
.build();
491+
492+
Pod combinedPod = combine(parentPod, childPod);
493+
assertEquals(asList(5001L, 5002L), combinedPod.getSpec().getSecurityContext().getSupplementalGroups());
494+
assertEquals(Long.valueOf(1000L), combinedPod.getSpec().getSecurityContext().getRunAsUser());
495+
assertEquals(Long.valueOf(1000L), combinedPod.getSpec().getSecurityContext().getRunAsGroup());
496+
}
497+
471498
@Test
472499
public void shouldFilterOutNullOrEmptyPodKeyValueEnvVars() {
473500
PodTemplate template1 = new PodTemplate();

‎src/test/java/org/csanchez/jenkins/plugins/kubernetes/SupplementalGroupsYamlOverrideReproTest.java‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
package org.csanchez.jenkins.plugins.kubernetes;
2+
3+
import static org.junit.Assert.assertEquals;
4+
import static org.junit.Assert.assertNotNull;
5+
import static org.junit.Assert.assertNull;
6+
import static org.mockito.Mockito.doReturn;
7+
import static org.mockito.Mockito.when;
8+
9+
import io.fabric8.kubernetes.api.model.Pod;
10+
import io.fabric8.kubernetes.api.model.PodSecurityContext;
11+
import java.util.Arrays;
12+
import org.csanchez.jenkins.plugins.kubernetes.pod.yaml.Overrides;
13+
import org.junit.Before;
14+
import org.junit.Rule;
15+
import org.junit.Test;
16+
import org.jvnet.hudson.test.JenkinsRule;
17+
import org.mockito.Mock;
18+
import org.mockito.Spy;
19+
import org.mockito.junit.MockitoJUnit;
20+
import org.mockito.junit.MockitoRule;
21+
22+
/**
23+
* Verifies supplementalGroups from pod template UI/CasC fields survive yaml override merge.
24+
* Related to jenkinsci/kubernetes-plugin#2444.
25+
*/
26+
public class SupplementalGroupsYamlOverrideReproTest {
27+
28+
private static final String AGENT_NAME = "jenkins-agent";
29+
private static final String AGENT_SECRET = "xxx";
30+
private static final String JENKINS_URL = "http://jenkins.example.com";
31+
32+
@Rule
33+
public JenkinsRule r = new JenkinsRule();
34+
35+
@Rule
36+
public MockitoRule mockitoRule = MockitoJUnit.rule();
37+
38+
@Spy
39+
private KubernetesCloud cloud = new KubernetesCloud("test");
40+
41+
@Mock
42+
private KubernetesSlave slave;
43+
44+
@Mock
45+
private KubernetesComputer computer;
46+
47+
@Before
48+
public void setUp() {
49+
when(slave.getKubernetesCloud()).thenReturn(cloud);
50+
doReturn(JENKINS_URL).when(cloud).getJenkinsUrlOrDie();
51+
when(computer.getName()).thenReturn(AGENT_NAME);
52+
when(computer.getJnlpMac()).thenReturn(AGENT_SECRET);
53+
when(slave.getComputer()).thenReturn(computer);
54+
}
55+
56+
@Test
57+
public void supplementalGroupsPresentWithoutRawYaml() throws Exception {
58+
PodTemplate template = baseTemplate();
59+
Pod pod = new PodTemplateBuilder(template, slave).build();
60+
61+
PodSecurityContext sc = pod.getSpec().getSecurityContext();
62+
assertNotNull(sc);
63+
assertEquals(Arrays.asList(5001L, 5002L), sc.getSupplementalGroups());
64+
}
65+
66+
@Test
67+
public void supplementalGroupsPreservedWhenRawYamlOverrideIsUsed() throws Exception {
68+
PodTemplate template = baseTemplate();
69+
template.setYamlMergeStrategy(new Overrides());
70+
template.setYaml(
71+
"""
72+
apiVersion: v1
73+
kind: Pod
74+
spec:
75+
containers:
76+
- name: jnlp
77+
image: jenkins/inbound-agent:latest
78+
command: ['cat']
79+
tty: true
80+
""");
81+
82+
Pod pod = new PodTemplateBuilder(template, slave).build();
83+
84+
PodSecurityContext sc = pod.getSpec().getSecurityContext();
85+
assertNotNull(sc);
86+
assertEquals(
87+
"expected supplementalGroups [5001, 5002] from pod template fields",
88+
Arrays.asList(5001L, 5002L),
89+
sc.getSupplementalGroups());
90+
}
91+
92+
private static PodTemplate baseTemplate() {
93+
PodTemplate template = new PodTemplate();
94+
template.setRunAsUser("1000");
95+
template.setRunAsGroup("1000");
96+
template.setSupplementalGroups("5001,5002");
97+
return template;
98+
}
99+
}

0 commit comments

Comments
 (0)