Skip to content

Commit 4d8d2c3

Browse files
author
Jim Minter
committed
don't alias Env slices in build controller strategy
1 parent 0c81307 commit 4d8d2c3

File tree

9 files changed

+80
-8
lines changed

9 files changed

+80
-8
lines changed

‎pkg/build/controller/strategy/custom_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ func TestCustomCreateBuildPod(t *testing.T) {
9797
t.Errorf("Expected %s variable to be set", name)
9898
}
9999
}
100+
101+
checkAliasing(t, actual)
100102
}
101103

102104
func TestCustomCreateBuildPodExpectedForcePull(t *testing.T) {

‎pkg/build/controller/strategy/docker.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
5555
Name: "docker-build",
5656
Image: bs.Image,
5757
Command: []string{"openshift-docker-build"},
58-
Env: containerEnv,
58+
Env: copyEnvVarSlice(containerEnv),
5959
// TODO: run unprivileged https://github.com/openshift/origin/issues/662
6060
SecurityContext: &v1.SecurityContext{
6161
Privileged: &privileged,
@@ -92,7 +92,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
9292
Name: GitCloneContainer,
9393
Image: bs.Image,
9494
Command: []string{"openshift-git-clone"},
95-
Env: containerEnv,
95+
Env: copyEnvVarSlice(containerEnv),
9696
TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
9797
VolumeMounts: []v1.VolumeMount{
9898
{
@@ -115,7 +115,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
115115
Name: ExtractImageContentContainer,
116116
Image: bs.Image,
117117
Command: []string{"openshift-extract-image-content"},
118-
Env: containerEnv,
118+
Env: copyEnvVarSlice(containerEnv),
119119
// TODO: run unprivileged https://github.com/openshift/origin/issues/662
120120
SecurityContext: &v1.SecurityContext{
121121
Privileged: &privileged,
@@ -137,7 +137,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
137137
Name: "manage-dockerfile",
138138
Image: bs.Image,
139139
Command: []string{"openshift-manage-dockerfile"},
140-
Env: containerEnv,
140+
Env: copyEnvVarSlice(containerEnv),
141141
TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
142142
VolumeMounts: []v1.VolumeMount{
143143
{

‎pkg/build/controller/strategy/docker_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ func TestDockerCreateBuildPod(t *testing.T) {
107107
t.Errorf("Expected %s:%s, got %s:%s!\n", exp[0], exp[1], e.Name, e.Value)
108108
}
109109
}
110+
111+
checkAliasing(t, actual)
110112
}
111113

112114
func TestDockerBuildLongName(t *testing.T) {

‎pkg/build/controller/strategy/sti.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
8080
Name: "sti-build",
8181
Image: bs.Image,
8282
Command: []string{"openshift-sti-build"},
83-
Env: containerEnv,
83+
Env: copyEnvVarSlice(containerEnv),
8484
// TODO: run unprivileged https://github.com/openshift/origin/issues/662
8585
SecurityContext: &v1.SecurityContext{
8686
Privileged: &privileged,
@@ -114,7 +114,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
114114
Name: GitCloneContainer,
115115
Image: bs.Image,
116116
Command: []string{"openshift-git-clone"},
117-
Env: containerEnv,
117+
Env: copyEnvVarSlice(containerEnv),
118118
TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
119119
VolumeMounts: []v1.VolumeMount{
120120
{
@@ -137,7 +137,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
137137
Name: ExtractImageContentContainer,
138138
Image: bs.Image,
139139
Command: []string{"openshift-extract-image-content"},
140-
Env: containerEnv,
140+
Env: copyEnvVarSlice(containerEnv),
141141
// TODO: run unprivileged https://github.com/openshift/origin/issues/662
142142
SecurityContext: &v1.SecurityContext{
143143
Privileged: &privileged,
@@ -159,7 +159,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
159159
Name: "manage-dockerfile",
160160
Image: bs.Image,
161161
Command: []string{"openshift-manage-dockerfile"},
162-
Env: containerEnv,
162+
Env: copyEnvVarSlice(containerEnv),
163163
TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
164164
VolumeMounts: []v1.VolumeMount{
165165
{

‎pkg/build/controller/strategy/sti_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,8 @@ func testSTICreateBuildPod(t *testing.T, rootAllowed bool) {
161161
t.Errorf("Expected %s:%s, got %s:%s!\n", exp[0], exp[1], e.Name, e.Value)
162162
}
163163
}
164+
165+
checkAliasing(t, actual)
164166
}
165167

166168
func TestS2IBuildLongName(t *testing.T) {

‎pkg/build/controller/strategy/util.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -265,3 +265,10 @@ func setOwnerReference(pod *v1.Pod, build *buildapi.Build) {
265265
},
266266
}
267267
}
268+
269+
// copyEnvVarSlice returns a copy of an []v1.EnvVar
270+
func copyEnvVarSlice(in []v1.EnvVar) []v1.EnvVar {
271+
out := make([]v1.EnvVar, len(in))
272+
copy(out, in)
273+
return out
274+
}

‎pkg/build/controller/strategy/util_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package strategy
22

33
import (
4+
"reflect"
45
"testing"
6+
"unsafe"
57

68
kapi "k8s.io/kubernetes/pkg/api"
79
"k8s.io/kubernetes/pkg/api/v1"
@@ -118,3 +120,36 @@ func TestSetupDockerSecrets(t *testing.T) {
118120
seenMountPath[m.Name] = true
119121
}
120122
}
123+
124+
func TestCopyEnvVarSlice(t *testing.T) {
125+
s1 := []v1.EnvVar{{Name: "FOO", Value: "bar"}, {Name: "BAZ", Value: "qux"}}
126+
s2 := copyEnvVarSlice(s1)
127+
128+
if !reflect.DeepEqual(s1, s2) {
129+
t.Error(s2)
130+
}
131+
132+
if (*reflect.SliceHeader)(unsafe.Pointer(&s1)).Data == (*reflect.SliceHeader)(unsafe.Pointer(&s2)).Data {
133+
t.Error("copyEnvVarSlice didn't copy backing store")
134+
}
135+
}
136+
137+
func checkAliasing(t *testing.T, pod *v1.Pod) {
138+
m := map[uintptr]bool{}
139+
for _, c := range pod.Spec.Containers {
140+
p := (*reflect.SliceHeader)(unsafe.Pointer(&c.Env)).Data
141+
if m[p] {
142+
t.Error("pod Env slices are aliased")
143+
return
144+
}
145+
m[p] = true
146+
}
147+
for _, c := range pod.Spec.InitContainers {
148+
p := (*reflect.SliceHeader)(unsafe.Pointer(&c.Env)).Data
149+
if m[p] {
150+
t.Error("pod Env slices are aliased")
151+
return
152+
}
153+
m[p] = true
154+
}
155+
}

‎test/extended/testdata/bindata.go

Lines changed: 12 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/extended/testdata/test-auth-build.yaml

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ parameters:
1010
- name: SOURCE_SECRET
1111
required: true
1212
objects:
13+
- apiVersion: v1
14+
kind: ImageStream
15+
metadata:
16+
name: output
1317
- apiVersion: v1
1418
kind: BuildConfig
1519
metadata:
@@ -28,3 +32,11 @@ objects:
2832
name: ruby:latest
2933
namespace: openshift
3034
type: Source
35+
# this test specifically does a push, to help exercise the code that sets
36+
# environment variables on build pods (i.e., by having a source secret and
37+
# a push secret, multiple environment variables need to be set correctly for
38+
# the build to succeed).
39+
output:
40+
to:
41+
kind: ImageStreamTag
42+
name: output:latest

0 commit comments

Comments
 (0)