Skip to content

Commit

Permalink
Move working dir initialization out to pkg/pod/
Browse files Browse the repository at this point in the history
This simplifies pod.go and makes this logic more easily
testable/understandable in isolation.

This also removes stepTemplate merging from init containers, where users
shouldn't know/care about the details of those containers anyway.

This makes some simplifying changes to creds-init as well, and adds
TODOs to further simplify.
  • Loading branch information
imjasonh authored and tekton-robot committed Nov 20, 2019
1 parent 5ef2ec0 commit 9fc4e0d
Show file tree
Hide file tree
Showing 5 changed files with 188 additions and 138 deletions.
67 changes: 67 additions & 0 deletions pkg/pod/workingdir_init.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package pod

import (
"path/filepath"
"sort"
"strings"

"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/names"
corev1 "k8s.io/api/core/v1"
)

const (
workspaceDir = "/workspace"
workingDirInit = "working-dir-initializer"
)

// WorkingDirInit returns a Container that should be run as an init
// container to ensure that all steps' workingDirs relative to the workspace
// exist.
//
// If no such directories need to be created (i.e., no relative workingDirs
// are specified), this method returns nil, as no init container is necessary.
//
// TODO(jasonhall): This should take []corev1.Container instead of
// []corev1.Step, but this makes it easier to use in pod.go. When pod.go is
// cleaned up, this can take []corev1.Container.
func WorkingDirInit(shellImage string, steps []v1alpha1.Step) *corev1.Container {
// Gather all unique workingDirs.
workingDirs := map[string]struct{}{}
for _, step := range steps {
if step.WorkingDir != "" {
workingDirs[step.WorkingDir] = struct{}{}
}
}
if len(workingDirs) == 0 {
return nil
}

// Sort unique workingDirs.
var orderedDirs []string
for wd := range workingDirs {
orderedDirs = append(orderedDirs, wd)
}
sort.Strings(orderedDirs)

// Clean and append each relative workingDir.
var relativeDirs []string
for _, wd := range orderedDirs {
p := filepath.Clean(wd)
if !filepath.IsAbs(p) || strings.HasPrefix(p, "/workspace/") {
relativeDirs = append(relativeDirs, p)
}
}

if len(relativeDirs) == 0 {
return nil
}

return &corev1.Container{
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(workingDirInit),
Image: shellImage,
Command: []string{"sh"},
Args: []string{"-c", "mkdir -p " + strings.Join(relativeDirs, " ")},
WorkingDir: workspaceDir,
}
}
69 changes: 69 additions & 0 deletions pkg/pod/workingdir_init_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
package pod

import (
"testing"

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/test/names"
corev1 "k8s.io/api/core/v1"
)

const shellImage = "shell-image"

func TestWorkingDirInit(t *testing.T) {
names.TestingSeed()
for _, c := range []struct {
desc string
stepContainers []corev1.Container
want *corev1.Container
}{{
desc: "no workingDirs",
stepContainers: []corev1.Container{{
Name: "no-working-dir",
}},
want: nil,
}, {
desc: "workingDirs are unique and sorted, absolute dirs are ignored",
stepContainers: []corev1.Container{{
WorkingDir: "zzz",
}, {
WorkingDir: "aaa",
}, {
WorkingDir: "/ignored",
}, {
WorkingDir: "/workspace", // ignored
}, {
WorkingDir: "zzz",
}, {
// Even though it's specified absolute, it's relative
// to /workspace, so we need to create it.
WorkingDir: "/workspace/bbb",
}},
want: &corev1.Container{
Name: "working-dir-initializer-9l9zj",
Image: shellImage,
Command: []string{"sh"},
Args: []string{"-c", "mkdir -p /workspace/bbb aaa zzz"},
WorkingDir: workspaceDir,
},
}} {
t.Run(c.desc, func(t *testing.T) {
// TODO(jasonhall): WorkingDirInit should take
// Containers instead of Steps, but while we're
// cleaning up pod.go it's easier to have it take
// Steps. This test doesn't care, so let's hide this
// conversion in the test where it's easier to remove
// later.
var steps []v1alpha1.Step
for _, c := range c.stepContainers {
steps = append(steps, v1alpha1.Step{Container: c})
}

got := WorkingDirInit(shellImage, steps)
if d := cmp.Diff(c.want, got); d != "" {
t.Fatalf("Diff (-want, +got): %s", d)
}
})
}
}
106 changes: 29 additions & 77 deletions pkg/reconciler/taskrun/resources/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"io"
"io/ioutil"
"path/filepath"
"sort"
"strings"

"github.com/tektoncd/pipeline/pkg/apis/pipeline"
Expand All @@ -34,6 +33,7 @@ import (
"github.com/tektoncd/pipeline/pkg/credentials/dockercreds"
"github.com/tektoncd/pipeline/pkg/credentials/gitcreds"
"github.com/tektoncd/pipeline/pkg/names"
"github.com/tektoncd/pipeline/pkg/pod"
"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/entrypoint"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -106,34 +106,36 @@ const (
containerPrefix = "step-"
unnamedInitContainerPrefix = "step-unnamed-"
// Name of the credential initialization container.
credsInit = "credential-initializer"
// Name of the working dir initialization container.
workingDirInit = "working-dir-initializer"
credsInit = "credential-initializer"
ReadyAnnotation = "tekton.dev/ready"
readyAnnotationValue = "READY"
sidecarPrefix = "sidecar-"
)

func makeCredentialInitializer(credsImage, serviceAccountName, namespace string, kubeclient kubernetes.Interface) (*v1alpha1.Step, []corev1.Volume, error) {
// TODO(jasonhall): If there are no creds to initialize, return a nil Container
// and don't add it to initContainers, instead of appending an init container
// that runs and does nothing.
func makeCredentialInitializer(credsImage, serviceAccountName, namespace string, kubeclient kubernetes.Interface) (corev1.Container, []corev1.Volume, error) {
if serviceAccountName == "" {
serviceAccountName = "default"
}

sa, err := kubeclient.CoreV1().ServiceAccounts(namespace).Get(serviceAccountName, metav1.GetOptions{})
if err != nil {
return nil, nil, err
return corev1.Container{}, nil, err
}

builders := []credentials.Builder{dockercreds.NewBuilder(), gitcreds.NewBuilder()}

// Collect the volume declarations, there mounts into the cred-init container, and the arguments to it.
volumes := []corev1.Volume{}
// Collect the volume declarations, there mounts into the cred-init
// container, and the arguments to it.
var volumes []corev1.Volume
volumeMounts := implicitVolumeMounts
args := []string{}
for _, secretEntry := range sa.Secrets {
secret, err := kubeclient.CoreV1().Secrets(namespace).Get(secretEntry.Name, metav1.GetOptions{})
if err != nil {
return nil, nil, err
return corev1.Container{}, nil, err
}

matched := false
Expand Down Expand Up @@ -161,87 +163,42 @@ func makeCredentialInitializer(credsImage, serviceAccountName, namespace string,
}
}

return &v1alpha1.Step{Container: corev1.Container{
return corev1.Container{
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(containerPrefix + credsInit),
Image: credsImage,
Command: []string{"/ko-app/creds-init"},
Args: args,
VolumeMounts: volumeMounts,
Env: implicitEnvVars,
WorkingDir: workspaceDir,
}}, volumes, nil
}

func makeWorkingDirScript(workingDirs map[string]bool) string {
script := ""
var orderedDirs []string

for wd := range workingDirs {
if wd != "" {
orderedDirs = append(orderedDirs, wd)
}
}
sort.Strings(orderedDirs)

for _, wd := range orderedDirs {
p := filepath.Clean(wd)
if rel, err := filepath.Rel(workspaceDir, p); err == nil && !strings.HasPrefix(rel, ".") {
if script == "" {
script = fmt.Sprintf("mkdir -p %s", p)
} else {
script = fmt.Sprintf("%s %s", script, p)
}
}
}

return script
}

func makeWorkingDirInitializer(shellImage string, steps []v1alpha1.Step) *v1alpha1.Step {
workingDirs := make(map[string]bool)
for _, step := range steps {
workingDirs[step.WorkingDir] = true
}

if script := makeWorkingDirScript(workingDirs); script != "" {
return &v1alpha1.Step{Container: corev1.Container{
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix(containerPrefix + workingDirInit),
Image: shellImage,
Command: []string{"sh"},
Args: []string{"-c", script},
VolumeMounts: implicitVolumeMounts,
Env: implicitEnvVars,
WorkingDir: workspaceDir,
}}
}
return nil
VolumeMounts: volumeMounts,
}, volumes, nil
}

// MakePod converts TaskRun and TaskSpec objects to a Pod which implements the taskrun specified
// by the supplied CRD.
func MakePod(images pipeline.Images, taskRun *v1alpha1.TaskRun, taskSpec v1alpha1.TaskSpec, kubeclient kubernetes.Interface) (*corev1.Pod, error) {
cred, secrets, err := makeCredentialInitializer(images.CredsImage, taskRun.GetServiceAccountName(), taskRun.Namespace, kubeclient)
credsInitContainer, secrets, err := makeCredentialInitializer(images.CredsImage, taskRun.GetServiceAccountName(), taskRun.Namespace, kubeclient)
if err != nil {
return nil, err
}
initSteps := []v1alpha1.Step{*cred}
var podSteps []v1alpha1.Step
initContainers := []corev1.Container{credsInitContainer}

if workingDir := makeWorkingDirInitializer(images.ShellImage, taskSpec.Steps); workingDir != nil {
initSteps = append(initSteps, *workingDir)
if workingDirInit := pod.WorkingDirInit(images.ShellImage, taskSpec.Steps); workingDirInit != nil {
initContainers = append(initContainers, *workingDirInit)
}

var podSteps []v1alpha1.Step

maxIndicesByResource := findMaxResourceRequest(taskSpec.Steps, corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage)

placeScripts := false
placeScriptsStep := v1alpha1.Step{Container: corev1.Container{
placeScriptsInitContainer := corev1.Container{
Name: names.SimpleNameGenerator.RestrictLengthWithRandomSuffix("place-scripts"),
Image: images.ShellImage,
TTY: true,
Command: []string{"sh"},
Args: []string{"-c", ""},
VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount},
}}
}

for i, s := range taskSpec.Steps {
s.Env = append(implicitEnvVars, s.Env...)
Expand Down Expand Up @@ -279,7 +236,7 @@ func MakePod(images pipeline.Images, taskRun *v1alpha1.TaskRun, taskSpec v1alpha
// interpreted as env vars and likely end up replaced
// with empty strings. See
// https://stackoverflow.com/a/27921346
placeScriptsStep.Args[1] += fmt.Sprintf(`tmpfile="%s"
placeScriptsInitContainer.Args[1] += fmt.Sprintf(`tmpfile="%s"
touch ${tmpfile} && chmod +x ${tmpfile}
cat > ${tmpfile} << '%s'
%s
Expand Down Expand Up @@ -315,8 +272,11 @@ cat > ${tmpfile} << '%s'
}
// use the container name to add the entrypoint binary as an
// init container.
// TODO(jasonhall): Entrypoint init container should be
// explicitly added as an init container, not added to steps
// then pulled out when translating to Pod containers.
if s.Name == names.SimpleNameGenerator.RestrictLength(fmt.Sprintf("%v%v", containerPrefix, entrypoint.InitContainerName)) {
initSteps = append(initSteps, s)
initContainers = append(initContainers, s.Container)
} else {
zeroNonMaxResourceRequests(&s, i, maxIndicesByResource)
podSteps = append(podSteps, s)
Expand All @@ -334,7 +294,7 @@ cat > ${tmpfile} << '%s'
// a script.
if placeScripts {
volumes = append(volumes, scriptsVolume)
initSteps = append(initSteps, placeScriptsStep)
initContainers = append(initContainers, placeScriptsInitContainer)
}

if err := v1alpha1.ValidateVolumes(volumes); err != nil {
Expand All @@ -348,14 +308,6 @@ cat > ${tmpfile} << '%s'
}
gibberish := hex.EncodeToString(b)

mergedInitSteps, err := v1alpha1.MergeStepsWithStepTemplate(taskSpec.StepTemplate, initSteps)
if err != nil {
return nil, err
}
var mergedInitContainers []corev1.Container
for _, s := range mergedInitSteps {
mergedInitContainers = append(mergedInitContainers, s.Container)
}
mergedPodSteps, err := v1alpha1.MergeStepsWithStepTemplate(taskSpec.StepTemplate, podSteps)
if err != nil {
return nil, err
Expand Down Expand Up @@ -388,7 +340,7 @@ cat > ${tmpfile} << '%s'
},
Spec: corev1.PodSpec{
RestartPolicy: corev1.RestartPolicyNever,
InitContainers: mergedInitContainers,
InitContainers: initContainers,
Containers: mergedPodContainers,
ServiceAccountName: taskRun.GetServiceAccountName(),
Volumes: volumes,
Expand Down
Loading

0 comments on commit 9fc4e0d

Please sign in to comment.