Skip to content

Commit 71cb99e

Browse files
openshift-merge-robotopenshift-merge-robot
authored
Merge pull request #15946 from jim-minter/issue15876
Automatic merge from submit-queue (batch tested with PRs 15942, 15940, 15957, 15858, 15946) allow secrets with "." characters to be used in builds fixes #15876
2 parents 01635f3 + 5e2badf commit 71cb99e

File tree

10 files changed

+133
-34
lines changed

10 files changed

+133
-34
lines changed

‎pkg/build/admission/buildpodutil.go

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,16 @@ import (
1515
// GetBuildFromPod returns a build object encoded in a pod's BUILD environment variable along with
1616
// its encoding version
1717
func GetBuildFromPod(pod *v1.Pod) (*buildapi.Build, schema.GroupVersion, error) {
18-
envVar, err := buildEnvVar(pod)
19-
if err != nil {
20-
return nil, schema.GroupVersion{}, fmt.Errorf("unable to get build from pod: %v", err)
18+
if len(pod.Spec.Containers) == 0 {
19+
return nil, schema.GroupVersion{}, errors.New("unable to get build from pod: pod has no containers")
20+
}
21+
22+
buildEnvVar := getEnvVar(&pod.Spec.Containers[0], "BUILD")
23+
if len(buildEnvVar) == 0 {
24+
return nil, schema.GroupVersion{}, errors.New("unable to get build from pod: BUILD environment variable is empty")
2125
}
22-
obj, groupVersionKind, err := kapi.Codecs.UniversalDecoder().Decode([]byte(envVar.Value), nil, nil)
26+
27+
obj, groupVersionKind, err := kapi.Codecs.UniversalDecoder().Decode([]byte(buildEnvVar), nil, nil)
2328
if err != nil {
2429
return nil, schema.GroupVersion{}, fmt.Errorf("unable to get build from pod: %v", err)
2530
}
@@ -32,15 +37,18 @@ func GetBuildFromPod(pod *v1.Pod) (*buildapi.Build, schema.GroupVersion, error)
3237

3338
// SetBuildInPod encodes a build object and sets it in a pod's BUILD environment variable
3439
func SetBuildInPod(pod *v1.Pod, build *buildapi.Build, groupVersion schema.GroupVersion) error {
35-
envVar, err := buildEnvVar(pod)
36-
if err != nil {
37-
return fmt.Errorf("unable to set build in pod: %v", err)
38-
}
3940
encodedBuild, err := runtime.Encode(kapi.Codecs.LegacyCodec(groupVersion), build)
4041
if err != nil {
4142
return fmt.Errorf("unable to set build in pod: %v", err)
4243
}
43-
envVar.Value = string(encodedBuild)
44+
45+
for i := range pod.Spec.Containers {
46+
setEnvVar(&pod.Spec.Containers[i], "BUILD", string(encodedBuild))
47+
}
48+
for i := range pod.Spec.InitContainers {
49+
setEnvVar(&pod.Spec.InitContainers[i], "BUILD", string(encodedBuild))
50+
}
51+
4452
return nil
4553
}
4654

@@ -79,25 +87,25 @@ func SetPodLogLevelFromBuild(pod *v1.Pod, build *buildapi.Build) error {
7987
return nil
8088
}
8189

82-
func buildEnvVar(pod *v1.Pod) (*v1.EnvVar, error) {
83-
if len(pod.Spec.Containers) == 0 {
84-
return nil, errors.New("pod has no containers")
85-
}
86-
env := pod.Spec.Containers[0].Env
87-
for i := range env {
88-
if env[i].Name == "BUILD" {
89-
if len(env[i].Value) == 0 {
90-
return nil, errors.New("BUILD environment variable is empty")
91-
}
92-
return &env[i], nil
90+
func getEnvVar(c *v1.Container, name string) string {
91+
for _, envVar := range c.Env {
92+
if envVar.Name == name {
93+
return envVar.Value
9394
}
9495
}
95-
return nil, errors.New("pod does not have a BUILD environment variable")
96+
97+
return ""
9698
}
9799

98-
func hasBuildEnvVar(pod *v1.Pod) bool {
99-
_, err := buildEnvVar(pod)
100-
return err == nil
100+
func setEnvVar(c *v1.Container, name, value string) {
101+
for i, envVar := range c.Env {
102+
if envVar.Name == name {
103+
c.Env[i] = v1.EnvVar{Name: name, Value: value}
104+
return
105+
}
106+
}
107+
108+
c.Env = append(c.Env, v1.EnvVar{Name: name, Value: value})
101109
}
102110

103111
func hasBuildAnnotation(pod *v1.Pod) bool {

‎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: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"fmt"
55
"path/filepath"
66
"strconv"
7+
"strings"
78

89
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
910
kvalidation "k8s.io/apimachinery/pkg/util/validation"
@@ -93,7 +94,11 @@ func setupDockerSocket(pod *v1.Pod) {
9394
// mountSecretVolume is a helper method responsible for actual mounting secret
9495
// volumes into a pod.
9596
func mountSecretVolume(pod *v1.Pod, container *v1.Container, secretName, mountPath, volumeSuffix string) {
96-
volumeName := namer.GetName(secretName, volumeSuffix, kvalidation.DNS1123SubdomainMaxLength)
97+
volumeName := namer.GetName(secretName, volumeSuffix, kvalidation.DNS1123LabelMaxLength)
98+
99+
// coerce from RFC1123 subdomain to RFC1123 label.
100+
volumeName = strings.Replace(volumeName, ".", "-", -1)
101+
97102
volumeExists := false
98103
for _, v := range pod.Spec.Volumes {
99104
if v.Name == volumeName {
@@ -265,3 +270,10 @@ func setOwnerReference(pod *v1.Pod, build *buildapi.Build) {
265270
},
266271
}
267272
}
273+
274+
// copyEnvVarSlice returns a copy of an []v1.EnvVar
275+
func copyEnvVarSlice(in []v1.EnvVar) []v1.EnvVar {
276+
out := make([]v1.EnvVar, len(in))
277+
copy(out, in)
278+
return out
279+
}

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

Lines changed: 50 additions & 1 deletion
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"
@@ -79,7 +81,7 @@ func TestSetupDockerSecrets(t *testing.T) {
7981
}
8082

8183
pushSecret := &kapi.LocalObjectReference{
82-
Name: "pushSecret",
84+
Name: "my.pushSecret.with.full.stops.and.longer.than.sixty.three.characters",
8385
}
8486
pullSecret := &kapi.LocalObjectReference{
8587
Name: "pullSecret",
@@ -104,6 +106,13 @@ func TestSetupDockerSecrets(t *testing.T) {
104106
seenName[v.Name] = true
105107
}
106108

109+
if !seenName["my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push"] {
110+
t.Errorf("volume my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push was not seen")
111+
}
112+
if !seenName["pullSecret-pull"] {
113+
t.Errorf("volume pullSecret-pull was not seen")
114+
}
115+
107116
seenMount := map[string]bool{}
108117
seenMountPath := map[string]bool{}
109118
for _, m := range pod.Spec.Containers[0].VolumeMounts {
@@ -117,4 +126,44 @@ func TestSetupDockerSecrets(t *testing.T) {
117126
}
118127
seenMountPath[m.Name] = true
119128
}
129+
130+
if !seenMount["my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push"] {
131+
t.Errorf("volumemount my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push was not seen")
132+
}
133+
if !seenMount["pullSecret-pull"] {
134+
t.Errorf("volumemount pullSecret-pull was not seen")
135+
}
136+
}
137+
138+
func TestCopyEnvVarSlice(t *testing.T) {
139+
s1 := []v1.EnvVar{{Name: "FOO", Value: "bar"}, {Name: "BAZ", Value: "qux"}}
140+
s2 := copyEnvVarSlice(s1)
141+
142+
if !reflect.DeepEqual(s1, s2) {
143+
t.Error(s2)
144+
}
145+
146+
if (*reflect.SliceHeader)(unsafe.Pointer(&s1)).Data == (*reflect.SliceHeader)(unsafe.Pointer(&s2)).Data {
147+
t.Error("copyEnvVarSlice didn't copy backing store")
148+
}
149+
}
150+
151+
func checkAliasing(t *testing.T, pod *v1.Pod) {
152+
m := map[uintptr]bool{}
153+
for _, c := range pod.Spec.Containers {
154+
p := (*reflect.SliceHeader)(unsafe.Pointer(&c.Env)).Data
155+
if m[p] {
156+
t.Error("pod Env slices are aliased")
157+
return
158+
}
159+
m[p] = true
160+
}
161+
for _, c := range pod.Spec.InitContainers {
162+
p := (*reflect.SliceHeader)(unsafe.Pointer(&c.Env)).Data
163+
if m[p] {
164+
t.Error("pod Env slices are aliased")
165+
return
166+
}
167+
m[p] = true
168+
}
120169
}

‎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)