allow secrets with "." characters to be used in builds

pkg/build/admission/buildpodutil.go

@@ -15,11 +15,16 @@ import (
 // GetBuildFromPod returns a build object encoded in a pod's BUILD environment variable along with
 // its encoding version
 func GetBuildFromPod(pod *v1.Pod) (*buildapi.Build, schema.GroupVersion, error) {
-	envVar, err := buildEnvVar(pod)
-	if err != nil {
-		return nil, schema.GroupVersion{}, fmt.Errorf("unable to get build from pod: %v", err)
+	if len(pod.Spec.Containers) == 0 {
+		return nil, schema.GroupVersion{}, errors.New("unable to get build from pod: pod has no containers")
+	}
+
+	buildEnvVar := getEnvVar(&pod.Spec.Containers[0], "BUILD")
+	if len(buildEnvVar) == 0 {
+		return nil, schema.GroupVersion{}, errors.New("unable to get build from pod: BUILD environment variable is empty")
 	}
-	obj, groupVersionKind, err := kapi.Codecs.UniversalDecoder().Decode([]byte(envVar.Value), nil, nil)
+
+	obj, groupVersionKind, err := kapi.Codecs.UniversalDecoder().Decode([]byte(buildEnvVar), nil, nil)
 	if err != nil {
 		return nil, schema.GroupVersion{}, fmt.Errorf("unable to get build from pod: %v", err)
 	}
@@ -32,15 +37,18 @@ func GetBuildFromPod(pod *v1.Pod) (*buildapi.Build, schema.GroupVersion, error)
 
 // SetBuildInPod encodes a build object and sets it in a pod's BUILD environment variable
 func SetBuildInPod(pod *v1.Pod, build *buildapi.Build, groupVersion schema.GroupVersion) error {
-	envVar, err := buildEnvVar(pod)
-	if err != nil {
-		return fmt.Errorf("unable to set build in pod: %v", err)
-	}
 	encodedBuild, err := runtime.Encode(kapi.Codecs.LegacyCodec(groupVersion), build)
 	if err != nil {
 		return fmt.Errorf("unable to set build in pod: %v", err)
 	}
-	envVar.Value = string(encodedBuild)
+
+	for i := range pod.Spec.Containers {
+		setEnvVar(&pod.Spec.Containers[i], "BUILD", string(encodedBuild))
+	}
+	for i := range pod.Spec.InitContainers {
+		setEnvVar(&pod.Spec.InitContainers[i], "BUILD", string(encodedBuild))
+	}
+
 	return nil
 }
 
@@ -79,25 +87,25 @@ func SetPodLogLevelFromBuild(pod *v1.Pod, build *buildapi.Build) error {
 	return nil
 }
 
-func buildEnvVar(pod *v1.Pod) (*v1.EnvVar, error) {
-	if len(pod.Spec.Containers) == 0 {
-		return nil, errors.New("pod has no containers")
-	}
-	env := pod.Spec.Containers[0].Env
-	for i := range env {
-		if env[i].Name == "BUILD" {
-			if len(env[i].Value) == 0 {
-				return nil, errors.New("BUILD environment variable is empty")
-			}
-			return &env[i], nil
+func getEnvVar(c *v1.Container, name string) string {
+	for _, envVar := range c.Env {
+		if envVar.Name == name {
+			return envVar.Value
 		}
 	}
-	return nil, errors.New("pod does not have a BUILD environment variable")
+
+	return ""
 }
 
-func hasBuildEnvVar(pod *v1.Pod) bool {
-	_, err := buildEnvVar(pod)
-	return err == nil
+func setEnvVar(c *v1.Container, name, value string) {
+	for i, envVar := range c.Env {
+		if envVar.Name == name {
+			c.Env[i] = v1.EnvVar{Name: name, Value: value}
+			return
+		}
+	}
+
+	c.Env = append(c.Env, v1.EnvVar{Name: name, Value: value})
 }
 
 func hasBuildAnnotation(pod *v1.Pod) bool {

pkg/build/controller/strategy/custom_test.go

@@ -97,6 +97,8 @@ func TestCustomCreateBuildPod(t *testing.T) {
 			t.Errorf("Expected %s variable to be set", name)
 		}
 	}
+
+	checkAliasing(t, actual)
 }
 
 func TestCustomCreateBuildPodExpectedForcePull(t *testing.T) {

pkg/build/controller/strategy/docker.go

@@ -55,7 +55,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 					Name:    "docker-build",
 					Image:   bs.Image,
 					Command: []string{"openshift-docker-build"},
-					Env:     containerEnv,
+					Env:     copyEnvVarSlice(containerEnv),
 					// TODO: run unprivileged https://github.com/openshift/origin/issues/662
 					SecurityContext: &v1.SecurityContext{
 						Privileged: &privileged,
@@ -92,7 +92,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 			Name:    GitCloneContainer,
 			Image:   bs.Image,
 			Command: []string{"openshift-git-clone"},
-			Env:     containerEnv,
+			Env:     copyEnvVarSlice(containerEnv),
 			TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
 			VolumeMounts: []v1.VolumeMount{
 				{
@@ -115,7 +115,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 			Name:    ExtractImageContentContainer,
 			Image:   bs.Image,
 			Command: []string{"openshift-extract-image-content"},
-			Env:     containerEnv,
+			Env:     copyEnvVarSlice(containerEnv),
 			// TODO: run unprivileged https://github.com/openshift/origin/issues/662
 			SecurityContext: &v1.SecurityContext{
 				Privileged: &privileged,
@@ -137,7 +137,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 			Name:    "manage-dockerfile",
 			Image:   bs.Image,
 			Command: []string{"openshift-manage-dockerfile"},
-			Env:     containerEnv,
+			Env:     copyEnvVarSlice(containerEnv),
 			TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
 			VolumeMounts: []v1.VolumeMount{
 				{

pkg/build/controller/strategy/docker_test.go

@@ -107,6 +107,8 @@ func TestDockerCreateBuildPod(t *testing.T) {
 			t.Errorf("Expected %s:%s, got %s:%s!\n", exp[0], exp[1], e.Name, e.Value)
 		}
 	}
+
+	checkAliasing(t, actual)
 }
 
 func TestDockerBuildLongName(t *testing.T) {

pkg/build/controller/strategy/sti.go

@@ -80,7 +80,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 					Name:    "sti-build",
 					Image:   bs.Image,
 					Command: []string{"openshift-sti-build"},
-					Env:     containerEnv,
+					Env:     copyEnvVarSlice(containerEnv),
 					// TODO: run unprivileged https://github.com/openshift/origin/issues/662
 					SecurityContext: &v1.SecurityContext{
 						Privileged: &privileged,
@@ -114,7 +114,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 			Name:    GitCloneContainer,
 			Image:   bs.Image,
 			Command: []string{"openshift-git-clone"},
-			Env:     containerEnv,
+			Env:     copyEnvVarSlice(containerEnv),
 			TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
 			VolumeMounts: []v1.VolumeMount{
 				{
@@ -137,7 +137,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 			Name:    ExtractImageContentContainer,
 			Image:   bs.Image,
 			Command: []string{"openshift-extract-image-content"},
-			Env:     containerEnv,
+			Env:     copyEnvVarSlice(containerEnv),
 			// TODO: run unprivileged https://github.com/openshift/origin/issues/662
 			SecurityContext: &v1.SecurityContext{
 				Privileged: &privileged,
@@ -159,7 +159,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e
 			Name:    "manage-dockerfile",
 			Image:   bs.Image,
 			Command: []string{"openshift-manage-dockerfile"},
-			Env:     containerEnv,
+			Env:     copyEnvVarSlice(containerEnv),
 			TerminationMessagePolicy: v1.TerminationMessageFallbackToLogsOnError,
 			VolumeMounts: []v1.VolumeMount{
 				{

pkg/build/controller/strategy/sti_test.go

@@ -161,6 +161,8 @@ func testSTICreateBuildPod(t *testing.T, rootAllowed bool) {
 			t.Errorf("Expected %s:%s, got %s:%s!\n", exp[0], exp[1], e.Name, e.Value)
 		}
 	}
+
+	checkAliasing(t, actual)
 }
 
 func TestS2IBuildLongName(t *testing.T) {

pkg/build/controller/strategy/util.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"path/filepath"
 	"strconv"
+	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kvalidation "k8s.io/apimachinery/pkg/util/validation"
@@ -93,7 +94,11 @@ func setupDockerSocket(pod *v1.Pod) {
 // mountSecretVolume is a helper method responsible for actual mounting secret
 // volumes into a pod.
 func mountSecretVolume(pod *v1.Pod, container *v1.Container, secretName, mountPath, volumeSuffix string) {
-	volumeName := namer.GetName(secretName, volumeSuffix, kvalidation.DNS1123SubdomainMaxLength)
+	volumeName := namer.GetName(secretName, volumeSuffix, kvalidation.DNS1123LabelMaxLength)
+
+	// coerce from RFC1123 subdomain to RFC1123 label.
+	volumeName = strings.Replace(volumeName, ".", "-", -1)
+
 	volumeExists := false
 	for _, v := range pod.Spec.Volumes {
 		if v.Name == volumeName {
@@ -265,3 +270,10 @@ func setOwnerReference(pod *v1.Pod, build *buildapi.Build) {
 		},
 	}
 }
+
+// copyEnvVarSlice returns a copy of an []v1.EnvVar
+func copyEnvVarSlice(in []v1.EnvVar) []v1.EnvVar {
+	out := make([]v1.EnvVar, len(in))
+	copy(out, in)
+	return out
+}

pkg/build/controller/strategy/util_test.go

@@ -1,7 +1,9 @@
 package strategy
 
 import (
+	"reflect"
 	"testing"
+	"unsafe"
 
 	kapi "k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/v1"
@@ -79,7 +81,7 @@ func TestSetupDockerSecrets(t *testing.T) {
 	}
 
 	pushSecret := &kapi.LocalObjectReference{
-		Name: "pushSecret",
+		Name: "my.pushSecret.with.full.stops.and.longer.than.sixty.three.characters",
 	}
 	pullSecret := &kapi.LocalObjectReference{
 		Name: "pullSecret",
@@ -104,6 +106,13 @@ func TestSetupDockerSecrets(t *testing.T) {
 		seenName[v.Name] = true
 	}
 
+	if !seenName["my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push"] {
+		t.Errorf("volume my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push was not seen")
+	}
+	if !seenName["pullSecret-pull"] {
+		t.Errorf("volume pullSecret-pull was not seen")
+	}
+
 	seenMount := map[string]bool{}
 	seenMountPath := map[string]bool{}
 	for _, m := range pod.Spec.Containers[0].VolumeMounts {
@@ -117,4 +126,44 @@ func TestSetupDockerSecrets(t *testing.T) {
 		}
 		seenMountPath[m.Name] = true
 	}
+
+	if !seenMount["my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push"] {
+		t.Errorf("volumemount my-pushSecret-with-full-stops-and-longer-than-six-c6eb4d75-push was not seen")
+	}
+	if !seenMount["pullSecret-pull"] {
+		t.Errorf("volumemount pullSecret-pull was not seen")
+	}
+}
+
+func TestCopyEnvVarSlice(t *testing.T) {
+	s1 := []v1.EnvVar{{Name: "FOO", Value: "bar"}, {Name: "BAZ", Value: "qux"}}
+	s2 := copyEnvVarSlice(s1)
+
+	if !reflect.DeepEqual(s1, s2) {
+		t.Error(s2)
+	}
+
+	if (*reflect.SliceHeader)(unsafe.Pointer(&s1)).Data == (*reflect.SliceHeader)(unsafe.Pointer(&s2)).Data {
+		t.Error("copyEnvVarSlice didn't copy backing store")
+	}
+}
+
+func checkAliasing(t *testing.T, pod *v1.Pod) {
+	m := map[uintptr]bool{}
+	for _, c := range pod.Spec.Containers {
+		p := (*reflect.SliceHeader)(unsafe.Pointer(&c.Env)).Data
+		if m[p] {
+			t.Error("pod Env slices are aliased")
+			return
+		}
+		m[p] = true
+	}
+	for _, c := range pod.Spec.InitContainers {
+		p := (*reflect.SliceHeader)(unsafe.Pointer(&c.Env)).Data
+		if m[p] {
+			t.Error("pod Env slices are aliased")
+			return
+		}
+		m[p] = true
+	}
 }

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

@@ -10,6 +10,10 @@ parameters:
 - name: SOURCE_SECRET
   required: true
 objects:
+- apiVersion: v1
+  kind: ImageStream
+  metadata:
+    name: output
 - apiVersion: v1
   kind: BuildConfig
   metadata:
@@ -28,3 +32,11 @@ objects:
           name: ruby:latest
           namespace: openshift
       type: Source
+    # this test specifically does a push, to help exercise the code that sets
+    # environment variables on build pods (i.e., by having a source secret and
+    # a push secret, multiple environment variables need to be set correctly for
+    # the build to succeed).
+    output:
+      to:
+        kind: ImageStreamTag
+        name: output:latest