pkg/pod: extract overrideHomeEnv/WorkingDir outside

This simplifies pkg/pod builder and simplify the work once we can remove this behavior altogether. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>
tektoncd · Oct 6, 2021 · 7a6604b · 7a6604b
1 parent 0603d01
commit 7a6604b
Show file tree

Hide file tree

Showing 6 changed files with 394 additions and 187 deletions.
diff --git a/pkg/internal/deprecated/override.go b/pkg/internal/deprecated/override.go
@@ -0,0 +1,82 @@
+/*
+Copyright 2021 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package deprecated
+
+import (
+	"context"
+
+	"github.com/tektoncd/pipeline/pkg/apis/config"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline"
+	"github.com/tektoncd/pipeline/pkg/pod"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func NewOverrideWorkingDirTransformer(ctx context.Context) pod.Transformer {
+	return func(p *corev1.Pod) (*corev1.Pod, error) {
+		if shouldOverrideWorkingDir(ctx) {
+			for i, c := range p.Spec.Containers {
+				if pod.IsContainerStep(c.Name) {
+					if c.WorkingDir == "" {
+						p.Spec.Containers[i].WorkingDir = pipeline.WorkspaceDir
+					}
+				}
+			}
+		}
+		return p, nil
+	}
+}
+
+// shouldOverrideWorkingDir returns a bool indicating whether a Pod should have its
+// working directory overwritten with /workspace or if it should be
+// left unmodified.
+//
+// For further reference see https://github.com/tektoncd/pipeline/issues/1836
+func shouldOverrideWorkingDir(ctx context.Context) bool {
+	cfg := config.FromContextOrDefaults(ctx)
+	return !cfg.FeatureFlags.DisableWorkingDirOverwrite
+}
+
+func NewOverrideHomeTransformer(ctx context.Context) pod.Transformer {
+	return func(p *corev1.Pod) (*corev1.Pod, error) {
+		if shouldOverrideHomeEnv(ctx) {
+			for i, c := range p.Spec.Containers {
+				hasHomeEnv := false
+				for _, e := range c.Env {
+					if e.Name == "HOME" {
+						hasHomeEnv = true
+					}
+				}
+				if !hasHomeEnv {
+					p.Spec.Containers[i].Env = append(p.Spec.Containers[i].Env, corev1.EnvVar{
+						Name:  "HOME",
+						Value: pipeline.HomeDir,
+					})
+				}
+			}
+		}
+		return p, nil
+	}
+}
+
+// shouldOverrideHomeEnv returns a bool indicating whether a Pod should have its
+// $HOME environment variable overwritten with /tekton/home or if it should be
+// left unmodified.
+//
+// For further reference see https://github.com/tektoncd/pipeline/issues/2013
+func shouldOverrideHomeEnv(ctx context.Context) bool {
+	cfg := config.FromContextOrDefaults(ctx)
+	return !cfg.FeatureFlags.DisableHomeEnvOverwrite
+}
diff --git a/pkg/internal/deprecated/override_test.go b/pkg/internal/deprecated/override_test.go
@@ -0,0 +1,309 @@
+/*
+Copyright 2021 The Tekton Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package deprecated_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/tektoncd/pipeline/pkg/apis/config"
+	"github.com/tektoncd/pipeline/pkg/internal/deprecated"
+	"github.com/tektoncd/pipeline/test/diff"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	logtesting "knative.dev/pkg/logging/testing"
+)
+
+const (
+	featureFlagDisableHomeEnvKey    = "disable-home-env-overwrite"
+	featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite"
+)
+
+func TestNewOverrideWorkingDirTransformer(t *testing.T) {
+
+	for _, tc := range []struct {
+		description string
+		configMap   *corev1.ConfigMap
+		podspec     corev1.PodSpec
+		expected    corev1.PodSpec
+	}{{
+		description: "Default behaviour: A missing disable-working-directory-overwrite should mean true, so no overwrite",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: "tekton-pipelines"},
+			Data:       map[string]string{},
+		},
+		podspec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+			}, {
+				Name:  "sidecar-bar",
+				Image: "foo",
+			}, {
+				Name:       "step-bar-wg",
+				Image:      "foo",
+				WorkingDir: "/foobar",
+			}},
+		},
+		expected: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+			}, {
+				Name:  "sidecar-bar",
+				Image: "foo",
+			}, {
+				Name:       "step-bar-wg",
+				Image:      "foo",
+				WorkingDir: "/foobar",
+			}},
+		},
+	}, {
+		description: "Setting disable-working-directory-overwrite to false should result in we don't disable the behavior, so there should be an overwrite",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: "tekton-pipelines"},
+			Data: map[string]string{
+				featureFlagDisableWorkingDirKey: "false",
+			},
+		},
+		podspec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+			}, {
+				Name:  "sidecar-bar",
+				Image: "foo",
+			}, {
+				Name:       "step-bar-wg",
+				Image:      "foo",
+				WorkingDir: "/foobar",
+			}},
+		},
+		expected: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:       "step-bar",
+				Image:      "foo",
+				WorkingDir: "/workspace",
+			}, {
+				Name:  "sidecar-bar",
+				Image: "foo",
+			}, {
+				Name:       "step-bar-wg",
+				Image:      "foo",
+				WorkingDir: "/foobar",
+			}},
+		},
+	}, {
+		description: "Setting disable-working-directory-overwrite to true should disable the overwrite, so no overwrite",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: "tekton-pipelines"},
+			Data: map[string]string{
+				featureFlagDisableWorkingDirKey: "true",
+			},
+		},
+		podspec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+			}, {
+				Name:  "sidecar-bar",
+				Image: "foo",
+			}, {
+				Name:       "step-bar-wg",
+				Image:      "foo",
+				WorkingDir: "/foobar",
+			}},
+		},
+		expected: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+			}, {
+				Name:  "sidecar-bar",
+				Image: "foo",
+			}, {
+				Name:       "step-bar-wg",
+				Image:      "foo",
+				WorkingDir: "/foobar",
+			}},
+		},
+	}} {
+		t.Run(tc.description, func(t *testing.T) {
+			store := config.NewStore(logtesting.TestLogger(t))
+			store.OnConfigChanged(tc.configMap)
+			ctx := store.ToContext(context.Background())
+			f := deprecated.NewOverrideWorkingDirTransformer(ctx)
+			got, err := f(&corev1.Pod{Spec: tc.podspec})
+			if err != nil {
+				t.Fatalf("Transformer failed: %v", err)
+			}
+			if d := cmp.Diff(tc.expected, got.Spec); d != "" {
+				t.Errorf("Diff pod: %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}
+
+func TestShouldOverrideHomeEnv(t *testing.T) {
+	for _, tc := range []struct {
+		description string
+		configMap   *corev1.ConfigMap
+		podspec     corev1.PodSpec
+		expected    corev1.PodSpec
+	}{{
+		description: "Default behaviour: A missing disable-home-env-overwrite flag should result in no overwrite",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: "tekton-pipelines"},
+			Data:       map[string]string{},
+		},
+		podspec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "HOME",
+					Value: "/home",
+				}},
+			}, {
+				Name:  "step-baz",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "FOO",
+					Value: "bar",
+				}},
+			}},
+		},
+		expected: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "HOME",
+					Value: "/home",
+				}},
+			}, {
+				Name:  "step-baz",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "FOO",
+					Value: "bar",
+				}},
+			}},
+		},
+	}, {
+		description: "Setting disable-home-env-overwrite to false should result in an overwrite",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: "tekton-pipelines"},
+			Data: map[string]string{
+				featureFlagDisableHomeEnvKey: "false",
+			},
+		},
+		podspec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "HOME",
+					Value: "/home",
+				}},
+			}, {
+				Name:  "step-baz",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "FOO",
+					Value: "bar",
+				}},
+			}},
+		},
+		expected: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "HOME",
+					Value: "/home",
+				}},
+			}, {
+				Name:  "step-baz",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "FOO",
+					Value: "bar",
+				}, {
+					Name:  "HOME",
+					Value: "/tekton/home",
+				}},
+			}},
+		},
+	}, {
+		description: "Setting disable-home-env-overwrite to true should result in no overwrite",
+		configMap: &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: "tekton-pipelines"},
+			Data: map[string]string{
+				featureFlagDisableHomeEnvKey: "true",
+			},
+		},
+		podspec: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "HOME",
+					Value: "/home",
+				}},
+			}, {
+				Name:  "step-baz",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "FOO",
+					Value: "bar",
+				}},
+			}},
+		},
+		expected: corev1.PodSpec{
+			Containers: []corev1.Container{{
+				Name:  "step-bar",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "HOME",
+					Value: "/home",
+				}},
+			}, {
+				Name:  "step-baz",
+				Image: "foo",
+				Env: []corev1.EnvVar{{
+					Name:  "FOO",
+					Value: "bar",
+				}},
+			}},
+		},
+	}} {
+		t.Run(tc.description, func(t *testing.T) {
+			store := config.NewStore(logtesting.TestLogger(t))
+			store.OnConfigChanged(tc.configMap)
+			ctx := store.ToContext(context.Background())
+			f := deprecated.NewOverrideHomeTransformer(ctx)
+			got, err := f(&corev1.Pod{Spec: tc.podspec})
+			if err != nil {
+				t.Fatalf("Transformer failed: %v", err)
+			}
+			if d := cmp.Diff(tc.expected, got.Spec); d != "" {
+				t.Errorf("Diff pod: %s", diff.PrintWantGot(d))
+			}
+		})
+	}
+}