From 10b2f9a07564f644e7c3edc9982a8c64eb781743 Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Fri, 15 May 2020 10:07:32 +0000 Subject: [PATCH 1/2] Add feature flags to config store Feature flags configuration is now tracked by the config store. This allows watching for the corresponding config map changes, removing the need to call kubernetes API to get the feature flags config map every time a pod is created. Signed-off-by: Arash Deshmeh --- pkg/apis/config/feature_flags.go | 90 +++++++++++++++ pkg/apis/config/feature_flags_test.go | 105 ++++++++++++++++++ pkg/apis/config/store.go | 24 +++- pkg/apis/config/store_test.go | 18 ++- .../testdata/feature-flags-all-flags-set.yaml | 24 ++++ .../config/testdata/feature-flags-empty.yaml | 26 +++++ pkg/apis/config/testdata/feature-flags.yaml | 24 ++++ pkg/apis/config/zz_generated.deepcopy.go | 16 +++ 8 files changed, 318 insertions(+), 9 deletions(-) create mode 100644 pkg/apis/config/feature_flags.go create mode 100644 pkg/apis/config/feature_flags_test.go create mode 100644 pkg/apis/config/testdata/feature-flags-all-flags-set.yaml create mode 100644 pkg/apis/config/testdata/feature-flags-empty.yaml create mode 100644 pkg/apis/config/testdata/feature-flags.yaml diff --git a/pkg/apis/config/feature_flags.go b/pkg/apis/config/feature_flags.go new file mode 100644 index 00000000000..97c507dcfe3 --- /dev/null +++ b/pkg/apis/config/feature_flags.go @@ -0,0 +1,90 @@ +/* +Copyright 2020 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 config + +import ( + "fmt" + "os" + "strconv" + + corev1 "k8s.io/api/core/v1" +) + +const ( + disableHomeEnvOverwriteKey = "disable-home-env-overwrite" + disableWorkingDirOverwriteKey = "disable-working-directory-overwrite" + disableAffinityAssistantKey = "disable-affinity-assistant" + runningInEnvWithInjectedSidecarsKey = "running-in-environment-with-injected-sidecars" + DefaultDisableHomeEnvOverwrite = false + DefaultDisableWorkingDirOverwrite = false + DefaultDisableAffinityAssistant = false + DefaultRunningInEnvWithInjectedSidecars = true +) + +// FeatureFlags holds the features configurations +// +k8s:deepcopy-gen=true +type FeatureFlags struct { + DisableHomeEnvOverwrite bool + DisableWorkingDirOverwrite bool + DisableAffinityAssistant bool + RunningInEnvWithInjectedSidecars bool +} + +// GetFeatureFlagsConfigName returns the name of the configmap containing all +// feature flags. +func GetFeatureFlagsConfigName() string { + if e := os.Getenv("CONFIG_FEATURE_FLAGS_NAME"); e != "" { + return e + } + return "feature-flags" +} + +// NewFeatureFlagsFromMap returns a Config given a map corresponding to a ConfigMap +func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) { + setFeature := func(key string, defaultValue bool, feature *bool) error { + if cfg, ok := cfgMap[key]; ok { + value, err := strconv.ParseBool(cfg) + if err != nil { + return fmt.Errorf("failed parsing feature flags config %q: %v", cfg, err) + } + *feature = value + return nil + } + *feature = defaultValue + return nil + } + + tc := FeatureFlags{} + if err := setFeature(disableHomeEnvOverwriteKey, DefaultDisableHomeEnvOverwrite, &tc.DisableHomeEnvOverwrite); err != nil { + return nil, err + } + if err := setFeature(disableWorkingDirOverwriteKey, DefaultDisableWorkingDirOverwrite, &tc.DisableWorkingDirOverwrite); err != nil { + return nil, err + } + if err := setFeature(disableAffinityAssistantKey, DefaultDisableAffinityAssistant, &tc.DisableAffinityAssistant); err != nil { + return nil, err + } + if err := setFeature(runningInEnvWithInjectedSidecarsKey, DefaultRunningInEnvWithInjectedSidecars, &tc.RunningInEnvWithInjectedSidecars); err != nil { + return nil, err + } + return &tc, nil +} + +// NewFeatureFlagsFromConfigMap returns a Config for the given configmap +func NewFeatureFlagsFromConfigMap(config *corev1.ConfigMap) (*FeatureFlags, error) { + return NewFeatureFlagsFromMap(config.Data) +} diff --git a/pkg/apis/config/feature_flags_test.go b/pkg/apis/config/feature_flags_test.go new file mode 100644 index 00000000000..695a205a882 --- /dev/null +++ b/pkg/apis/config/feature_flags_test.go @@ -0,0 +1,105 @@ +/* +Copyright 2020 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 config + +import ( + "os" + "testing" + + "github.com/google/go-cmp/cmp" + test "github.com/tektoncd/pipeline/pkg/reconciler/testing" + "github.com/tektoncd/pipeline/test/diff" +) + +func TestNewFeatureFlagsFromConfigMap(t *testing.T) { + type testCase struct { + expectedConfig *FeatureFlags + fileName string + } + + testCases := []testCase{ + { + expectedConfig: &FeatureFlags{ + RunningInEnvWithInjectedSidecars: DefaultRunningInEnvWithInjectedSidecars, + }, + fileName: GetFeatureFlagsConfigName(), + }, + { + expectedConfig: &FeatureFlags{ + DisableHomeEnvOverwrite: true, + DisableWorkingDirOverwrite: true, + DisableAffinityAssistant: true, + RunningInEnvWithInjectedSidecars: false, + }, + fileName: "feature-flags-all-flags-set", + }, + } + + for _, tc := range testCases { + verifyConfigFileWithExpectedFeatureFlagsConfig(t, tc.fileName, tc.expectedConfig) + } +} + +func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) { + FeatureFlagsConfigEmptyName := "feature-flags-empty" + expectedConfig := &FeatureFlags{ + RunningInEnvWithInjectedSidecars: true, + } + verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig) +} + +func TestGetFeatureFlagsConfigName(t *testing.T) { + for _, tc := range []struct { + description string + featureFlagEnvValue string + expected string + }{{ + description: "Feature flags config value not set", + featureFlagEnvValue: "", + expected: "feature-flags", + }, { + description: "Feature flags config value set", + featureFlagEnvValue: "feature-flags-test", + expected: "feature-flags-test", + }} { + t.Run(tc.description, func(t *testing.T) { + original := os.Getenv("CONFIG_FEATURE_FLAGS_NAME") + defer t.Cleanup(func() { + os.Setenv("CONFIG_FEATURE_FLAGS_NAME", original) + }) + if tc.featureFlagEnvValue != "" { + os.Setenv("CONFIG_FEATURE_FLAGS_NAME", tc.featureFlagEnvValue) + } + got := GetFeatureFlagsConfigName() + want := tc.expected + if got != want { + t.Errorf("GetFeatureFlagsConfigName() = %s, want %s", got, want) + } + }) + } +} + +func verifyConfigFileWithExpectedFeatureFlagsConfig(t *testing.T, fileName string, expectedConfig *FeatureFlags) { + cm := test.ConfigMapFromTestFile(t, fileName) + if flags, err := NewFeatureFlagsFromConfigMap(cm); err == nil { + if d := cmp.Diff(flags, expectedConfig); d != "" { + t.Errorf("Diff:\n%s", diff.PrintWantGot(d)) + } + } else { + t.Errorf("NewFeatureFlagsFromConfigMap(actual) = %v", err) + } +} diff --git a/pkg/apis/config/store.go b/pkg/apis/config/store.go index a7423ca3bcc..9b5c87331d5 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -27,7 +27,8 @@ type cfgKey struct{} // Config holds the collection of configurations that we attach to contexts. // +k8s:deepcopy-gen=false type Config struct { - Defaults *Defaults + Defaults *Defaults + FeatureFlags *FeatureFlags } // FromContext extracts a Config from the provided context. @@ -46,8 +47,10 @@ func FromContextOrDefaults(ctx context.Context) *Config { return cfg } defaults, _ := NewDefaultsFromMap(map[string]string{}) + featureFlags, _ := NewFeatureFlagsFromMap(map[string]string{}) return &Config{ - Defaults: defaults, + Defaults: defaults, + FeatureFlags: featureFlags, } } @@ -67,10 +70,11 @@ type Store struct { func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store { store := &Store{ UntypedStore: configmap.NewUntypedStore( - "defaults", + "defaults/features", logger, configmap.Constructors{ - GetDefaultsConfigName(): NewDefaultsFromConfigMap, + GetDefaultsConfigName(): NewDefaultsFromConfigMap, + GetFeatureFlagsConfigName(): NewFeatureFlagsFromConfigMap, }, onAfterStore..., ), @@ -86,7 +90,17 @@ func (s *Store) ToContext(ctx context.Context) context.Context { // Load creates a Config from the current config state of the Store. func (s *Store) Load() *Config { + defaults := s.UntypedLoad(GetDefaultsConfigName()) + if defaults == nil { + defaults, _ = NewDefaultsFromMap(map[string]string{}) + } + featureFlags := s.UntypedLoad(GetFeatureFlagsConfigName()) + if featureFlags == nil { + featureFlags, _ = NewFeatureFlagsFromMap(map[string]string{}) + } + return &Config{ - Defaults: s.UntypedLoad(GetDefaultsConfigName()).(*Defaults).DeepCopy(), + Defaults: defaults.(*Defaults).DeepCopy(), + FeatureFlags: featureFlags.(*FeatureFlags).DeepCopy(), } } diff --git a/pkg/apis/config/store_test.go b/pkg/apis/config/store_test.go index 46f9e9c59b3..fcd3c59fb17 100644 --- a/pkg/apis/config/store_test.go +++ b/pkg/apis/config/store_test.go @@ -27,14 +27,24 @@ import ( ) func TestStoreLoadWithContext(t *testing.T) { - store := NewStore(logtesting.TestLogger(t)) defaultConfig := test.ConfigMapFromTestFile(t, "config-defaults") + featuresConfig := test.ConfigMapFromTestFile(t, "feature-flags-all-flags-set") + + expectedDefaults, _ := NewDefaultsFromConfigMap(defaultConfig) + expectedFeatures, _ := NewFeatureFlagsFromConfigMap(featuresConfig) + + expected := &Config{ + Defaults: expectedDefaults, + FeatureFlags: expectedFeatures, + } + + store := NewStore(logtesting.TestLogger(t)) store.OnConfigChanged(defaultConfig) + store.OnConfigChanged(featuresConfig) config := FromContext(store.ToContext(context.Background())) - expected, _ := NewDefaultsFromConfigMap(defaultConfig) - if d := cmp.Diff(config.Defaults, expected); d != "" { - t.Errorf("Unexpected default config %s", diff.PrintWantGot(d)) + if d := cmp.Diff(config, expected); d != "" { + t.Errorf("Unexpected config %s", diff.PrintWantGot(d)) } } diff --git a/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml new file mode 100644 index 00000000000..c940a69e5f7 --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-all-flags-set.yaml @@ -0,0 +1,24 @@ +# Copyright 2020 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 +# +# https://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. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + disable-home-env-overwrite: "true" + disable-working-directory-overwrite: "true" + disable-affinity-assistant: "true" + running-in-environment-with-injected-sidecars: "false" diff --git a/pkg/apis/config/testdata/feature-flags-empty.yaml b/pkg/apis/config/testdata/feature-flags-empty.yaml new file mode 100644 index 00000000000..df625b56b33 --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags-empty.yaml @@ -0,0 +1,26 @@ +# Copyright 2020 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 +# +# https://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. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + _example: | + ################################ + # # + # EXAMPLE CONFIGURATION # + # # + ################################ diff --git a/pkg/apis/config/testdata/feature-flags.yaml b/pkg/apis/config/testdata/feature-flags.yaml new file mode 100644 index 00000000000..2dcd768dde0 --- /dev/null +++ b/pkg/apis/config/testdata/feature-flags.yaml @@ -0,0 +1,24 @@ +# Copyright 2020 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 +# +# https://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. + +apiVersion: v1 +kind: ConfigMap +metadata: + name: feature-flags + namespace: tekton-pipelines +data: + disable-home-env-overwrite: "false" + disable-working-directory-overwrite: "false" + disable-affinity-assistant: "false" + running-in-environment-with-injected-sidecars: "true" diff --git a/pkg/apis/config/zz_generated.deepcopy.go b/pkg/apis/config/zz_generated.deepcopy.go index 58a6d7a288b..f17d31c89df 100644 --- a/pkg/apis/config/zz_generated.deepcopy.go +++ b/pkg/apis/config/zz_generated.deepcopy.go @@ -44,3 +44,19 @@ func (in *Defaults) DeepCopy() *Defaults { in.DeepCopyInto(out) return out } + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *FeatureFlags) DeepCopyInto(out *FeatureFlags) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new FeatureFlags. +func (in *FeatureFlags) DeepCopy() *FeatureFlags { + if in == nil { + return nil + } + out := new(FeatureFlags) + in.DeepCopyInto(out) + return out +} From 64da2b522e2cb81eb3531649bb9cdce26c7fdb21 Mon Sep 17 00:00:00 2001 From: Arash Deshmeh Date: Sat, 16 May 2020 21:59:13 +0000 Subject: [PATCH 2/2] Use configmap watcher for feature flags config Taskrun reconciler now uses a configmap watcher for `feature-flags` configmap to remove the need for an extra Kubernetes API call to get the configmap's value every time a pod is being created. Signed-off-by: Arash Deshmeh --- pkg/pod/pod.go | 52 +--- pkg/pod/pod_test.go | 92 +++---- .../pipelinerun/affinity_assistant.go | 13 +- .../pipelinerun/affinity_assistant_test.go | 14 +- pkg/reconciler/pipelinerun/pipelinerun.go | 8 +- pkg/reconciler/taskrun/controller.go | 3 + pkg/reconciler/taskrun/taskrun.go | 17 +- pkg/reconciler/taskrun/taskrun_test.go | 258 ++++++++++++++++-- test/controller.go | 12 + .../informers/core/v1/configmap/configmap.go | 52 ++++ .../informers/core/v1/configmap/fake/fake.go | 40 +++ vendor/modules.txt | 2 + 12 files changed, 429 insertions(+), 134 deletions(-) create mode 100644 vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/configmap.go create mode 100644 vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake/fake.go diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index 9917895f0c4..7e4839b3e84 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -17,14 +17,14 @@ limitations under the License. package pod import ( + "context" "fmt" - "os" "path/filepath" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/names" - "github.com/tektoncd/pipeline/pkg/system" "github.com/tektoncd/pipeline/pkg/version" "github.com/tektoncd/pipeline/pkg/workspace" corev1 "k8s.io/api/core/v1" @@ -33,25 +33,10 @@ import ( "k8s.io/client-go/kubernetes" ) -// GetFeatureFlagsConfigName returns the name of the configmap containing all -// customizations for the feature flags. -func GetFeatureFlagsConfigName() string { - if e := os.Getenv("CONFIG_FEATURE_FLAGS_NAME"); e != "" { - return e - } - return "feature-flags" -} - const ( // ResultsDir is the folder used by default to create the results file ResultsDir = "/tekton/results" - featureInjectedSidecar = "running-in-environment-with-injected-sidecars" - featureFlagDisableHomeEnvKey = "disable-home-env-overwrite" - featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite" - featureFlagConfigMapName = "feature-flags" - featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create" - taskRunLabelKey = pipeline.GroupName + pipeline.TaskRunLabelKey ) @@ -90,7 +75,7 @@ var ( // MakePod converts TaskRun and TaskSpec objects to a Pod which implements the taskrun specified // by the supplied CRD. -func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.TaskSpec, kubeclient kubernetes.Interface, entrypointCache EntrypointCache, overrideHomeEnv bool) (*corev1.Pod, error) { +func MakePod(ctx context.Context, images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1.TaskSpec, kubeclient kubernetes.Interface, entrypointCache EntrypointCache, overrideHomeEnv bool) (*corev1.Pod, error) { var initContainers []corev1.Container var volumes []corev1.Volume var volumeMounts []corev1.VolumeMount @@ -194,7 +179,7 @@ func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1. // - sets container name to add "step-" prefix or "step-unnamed-#" if not specified. // TODO(#1605): Remove this loop and make each transformation in // isolation. - shouldOverrideWorkingDir := shouldOverrideWorkingDir(kubeclient) + shouldOverrideWorkingDir := shouldOverrideWorkingDir(ctx) for i, s := range stepContainers { if s.WorkingDir == "" && shouldOverrideWorkingDir { stepContainers[i].WorkingDir = pipeline.WorkspaceDir @@ -253,7 +238,7 @@ func MakePod(images pipeline.Images, taskRun *v1beta1.TaskRun, taskSpec v1beta1. podAnnotations := taskRun.Annotations podAnnotations[ReleaseAnnotation] = ReleaseAnnotationValue - if shouldAddReadyAnnotationOnPodCreate(taskSpec.Sidecars, kubeclient) { + if shouldAddReadyAnnotationOnPodCreate(ctx, taskSpec.Sidecars) { podAnnotations[readyAnnotation] = readyAnnotationValue } @@ -369,12 +354,9 @@ func getLimitRangeMinimum(namespace string, kubeclient kubernetes.Interface) (co // but this is planned to change in an upcoming release. // // For further reference see https://github.com/tektoncd/pipeline/issues/2013 -func ShouldOverrideHomeEnv(kubeclient kubernetes.Interface) bool { - configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{}) - if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagDisableHomeEnvKey] == "true" { - return false - } - return true +func ShouldOverrideHomeEnv(ctx context.Context) bool { + cfg := config.FromContextOrDefaults(ctx) + return !cfg.FeatureFlags.DisableHomeEnvOverwrite } // shouldOverrideWorkingDir returns a bool indicating whether a Pod should have its @@ -383,28 +365,22 @@ func ShouldOverrideHomeEnv(kubeclient kubernetes.Interface) bool { // if not specified by the user, but this is planned to change in an upcoming release. // // For further reference see https://github.com/tektoncd/pipeline/issues/1836 -func shouldOverrideWorkingDir(kubeclient kubernetes.Interface) bool { - configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{}) - if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagDisableWorkingDirKey] == "true" { - return false - } - return true +func shouldOverrideWorkingDir(ctx context.Context) bool { + cfg := config.FromContextOrDefaults(ctx) + return !cfg.FeatureFlags.DisableWorkingDirOverwrite } // shouldAddReadyAnnotationonPodCreate returns a bool indicating whether the // controller should add the `Ready` annotation when creating the Pod. We cannot // add the annotation if Tekton is running in a cluster with injected sidecars // or if the Task specifies any sidecars. -func shouldAddReadyAnnotationOnPodCreate(sidecars []v1beta1.Sidecar, kubeclient kubernetes.Interface) bool { +func shouldAddReadyAnnotationOnPodCreate(ctx context.Context, sidecars []v1beta1.Sidecar) bool { // If the TaskRun has sidecars, we cannot set the READY annotation early if len(sidecars) > 0 { return false } // If the TaskRun has no sidecars, check if we are running in a cluster where sidecars can be injected by other // controllers. - configMap, err := kubeclient.CoreV1().ConfigMaps(system.GetNamespace()).Get(GetFeatureFlagsConfigName(), metav1.GetOptions{}) - if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureInjectedSidecar] == "false" { - return true - } - return false + cfg := config.FromContextOrDefaults(ctx) + return !cfg.FeatureFlags.RunningInEnvWithInjectedSidecars } diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go index 77110ee86f5..43864586b2d 100644 --- a/pkg/pod/pod_test.go +++ b/pkg/pod/pod_test.go @@ -17,14 +17,15 @@ limitations under the License. package pod import ( + "context" "fmt" - "os" "path/filepath" "strings" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" @@ -35,6 +36,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakek8s "k8s.io/client-go/kubernetes/fake" + logtesting "knative.dev/pkg/logging/testing" ) var ( @@ -47,6 +49,10 @@ var ( ignoreReleaseAnnotation = func(k string, v string) bool { return k == ReleaseAnnotation } + featureInjectedSidecar = "running-in-environment-with-injected-sidecars" + featureFlagDisableHomeEnvKey = "disable-home-env-overwrite" + featureFlagDisableWorkingDirKey = "disable-working-directory-overwrite" + featureFlagSetReadyAnnotationOnPodCreate = "enable-ready-annotation-on-pod-create" ) func TestMakePod(t *testing.T) { @@ -980,11 +986,14 @@ script-heredoc-randomly-generated-78c5n }}} { t.Run(c.desc, func(t *testing.T) { names.TestingSeed() - kubeclient := fakek8s.NewSimpleClientset( + store := config.NewStore(logtesting.TestLogger(t)) + store.OnConfigChanged( &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: featureFlagConfigMapName, Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: c.featureFlags, }, + ) + kubeclient := fakek8s.NewSimpleClientset( &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "default", Namespace: "default"}}, &corev1.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: "service-account", Namespace: "default"}, Secrets: []corev1.ObjectReference{{ @@ -1026,7 +1035,7 @@ script-heredoc-randomly-generated-78c5n // No entrypoints should be looked up. entrypointCache := fakeCache{} - got, err := MakePod(images, tr, c.ts, kubeclient, entrypointCache, true) + got, err := MakePod(store.ToContext(context.Background()), images, tr, c.ts, kubeclient, entrypointCache, true) if err != nil { t.Fatalf("MakePod: %v", err) } @@ -1077,14 +1086,14 @@ func TestShouldOverrideHomeEnv(t *testing.T) { }{{ description: "Default behaviour: A missing disable-home-env-overwrite flag should result in true", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{}, }, expected: true, }, { description: "Setting disable-home-env-overwrite to false should result in true", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureFlagDisableHomeEnvKey: "false", }, @@ -1093,7 +1102,7 @@ func TestShouldOverrideHomeEnv(t *testing.T) { }, { description: "Setting disable-home-env-overwrite to true should result in false", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureFlagDisableHomeEnvKey: "true", }, @@ -1101,47 +1110,15 @@ func TestShouldOverrideHomeEnv(t *testing.T) { expected: false, }} { t.Run(tc.description, func(t *testing.T) { - kubeclient := fakek8s.NewSimpleClientset( - tc.configMap, - ) - if result := ShouldOverrideHomeEnv(kubeclient); result != tc.expected { + store := config.NewStore(logtesting.TestLogger(t)) + store.OnConfigChanged(tc.configMap) + if result := ShouldOverrideHomeEnv(store.ToContext(context.Background())); result != tc.expected { t.Errorf("Expected %t Received %t", tc.expected, result) } }) } } -func TestGetFeatureFlagsConfigName(t *testing.T) { - for _, tc := range []struct { - description string - featureFlagEnvValue string - expected string - }{{ - description: "Feature flags config value not set", - featureFlagEnvValue: "", - expected: "feature-flags", - }, { - description: "Feature flags config value set", - featureFlagEnvValue: "feature-flags-test", - expected: "feature-flags-test", - }} { - t.Run(tc.description, func(t *testing.T) { - original := os.Getenv("CONFIG_FEATURE_FLAGS_NAME") - defer t.Cleanup(func() { - os.Setenv("CONFIG_FEATURE_FLAGS_NAME", original) - }) - if tc.featureFlagEnvValue != "" { - os.Setenv("CONFIG_FEATURE_FLAGS_NAME", tc.featureFlagEnvValue) - } - got := GetFeatureFlagsConfigName() - want := tc.expected - if got != want { - t.Errorf("GetFeatureFlagsConfigName() = %s, want %s", got, want) - } - }) - } -} - func TestShouldOverrideWorkingDir(t *testing.T) { for _, tc := range []struct { description string @@ -1150,14 +1127,14 @@ func TestShouldOverrideWorkingDir(t *testing.T) { }{{ description: "Default behaviour: A missing disable-working-directory-overwrite flag should result in true", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{}, }, expected: true, }, { description: "Setting disable-working-directory-overwrite to false should result in true", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureFlagDisableWorkingDirKey: "false", }, @@ -1166,7 +1143,7 @@ func TestShouldOverrideWorkingDir(t *testing.T) { }, { description: "Setting disable-working-directory-overwrite to true should result in false", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureFlagDisableWorkingDirKey: "true", }, @@ -1174,10 +1151,10 @@ func TestShouldOverrideWorkingDir(t *testing.T) { expected: false, }} { t.Run(tc.description, func(t *testing.T) { - kubeclient := fakek8s.NewSimpleClientset( - tc.configMap, - ) - if result := shouldOverrideWorkingDir(kubeclient); result != tc.expected { + store := config.NewStore(logtesting.TestLogger(t)) + store.OnConfigChanged(tc.configMap) + ctx := store.ToContext(context.Background()) + if result := shouldOverrideWorkingDir(ctx); result != tc.expected { t.Errorf("Expected %t Received %t", tc.expected, result) } }) @@ -1199,7 +1176,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { description: "Default behavior with sidecars present: Ready annotation not set on pod create", sidecars: []v1beta1.Sidecar{sd}, configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{}, }, expected: false, @@ -1207,7 +1184,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { description: "Default behavior with no sidecars present: Ready annotation not set on pod create", sidecars: []v1beta1.Sidecar{}, configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{}, }, expected: false, @@ -1215,7 +1192,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { description: "Setting running-in-environment-with-injected-sidecars to true with sidecars present results in false", sidecars: []v1beta1.Sidecar{sd}, configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureInjectedSidecar: "true", }, @@ -1225,7 +1202,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { description: "Setting running-in-environment-with-injected-sidecars to true with no sidecars present results in false", sidecars: []v1beta1.Sidecar{}, configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureInjectedSidecar: "true", }, @@ -1235,7 +1212,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { description: "Setting running-in-environment-with-injected-sidecars to false with sidecars present results in false", sidecars: []v1beta1.Sidecar{sd}, configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureInjectedSidecar: "false", }, @@ -1245,7 +1222,7 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { description: "Setting running-in-environment-with-injected-sidecars to false with no sidecars present results in true", sidecars: []v1beta1.Sidecar{}, configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureInjectedSidecar: "false", }, @@ -1255,8 +1232,9 @@ func TestShouldAddReadyAnnotationonPodCreate(t *testing.T) { for _, tc := range tcs { t.Run(tc.description, func(t *testing.T) { - kubclient := fakek8s.NewSimpleClientset(tc.configMap) - if result := shouldAddReadyAnnotationOnPodCreate(tc.sidecars, kubclient); result != tc.expected { + store := config.NewStore(logtesting.TestLogger(t)) + store.OnConfigChanged(tc.configMap) + if result := shouldAddReadyAnnotationOnPodCreate(store.ToContext(context.Background()), tc.sidecars); result != tc.expected { t.Errorf("expected: %t Received: %t", tc.expected, result) } }) diff --git a/pkg/reconciler/pipelinerun/affinity_assistant.go b/pkg/reconciler/pipelinerun/affinity_assistant.go index b3db5fa72e6..b6920b9cc43 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant.go @@ -17,14 +17,14 @@ limitations under the License. package pipelinerun import ( + "context" "fmt" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" - "github.com/tektoncd/pipeline/pkg/pod" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" - "github.com/tektoncd/pipeline/pkg/system" "github.com/tektoncd/pipeline/pkg/workspace" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -217,10 +217,7 @@ func affinityAssistantStatefulSet(name string, pr *v1beta1.PipelineRun, claimNam // be created for each PipelineRun that use workspaces with PersistentVolumeClaims // as volume source. The default behaviour is to enable the Affinity Assistant to // provide Node Affinity for TaskRuns that share a PVC workspace. -func (c *Reconciler) isAffinityAssistantDisabled() bool { - configMap, err := c.KubeClientSet.CoreV1().ConfigMaps(system.GetNamespace()).Get(pod.GetFeatureFlagsConfigName(), metav1.GetOptions{}) - if err == nil && configMap != nil && configMap.Data != nil && configMap.Data[featureFlagDisableAffinityAssistantKey] == "true" { - return true - } - return false +func (c *Reconciler) isAffinityAssistantDisabled(ctx context.Context) bool { + cfg := config.FromContextOrDefaults(ctx) + return cfg.FeatureFlags.DisableAffinityAssistant } diff --git a/pkg/reconciler/pipelinerun/affinity_assistant_test.go b/pkg/reconciler/pipelinerun/affinity_assistant_test.go index 7ed17023c1f..b499fe3ea35 100644 --- a/pkg/reconciler/pipelinerun/affinity_assistant_test.go +++ b/pkg/reconciler/pipelinerun/affinity_assistant_test.go @@ -17,12 +17,13 @@ limitations under the License. package pipelinerun import ( + "context" "fmt" "testing" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" - "github.com/tektoncd/pipeline/pkg/pod" "github.com/tektoncd/pipeline/pkg/reconciler" "github.com/tektoncd/pipeline/pkg/system" "go.uber.org/zap" @@ -30,6 +31,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakek8s "k8s.io/client-go/kubernetes/fake" + logtesting "knative.dev/pkg/logging/testing" ) // TestCreateAndDeleteOfAffinityAssistant tests to create and delete an Affinity Assistant @@ -142,14 +144,14 @@ func TestDisableAffinityAssistant(t *testing.T) { }{{ description: "Default behaviour: A missing disable-affinity-assistant flag should result in false", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: pod.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{}, }, expected: false, }, { description: "Setting disable-affinity-assistant to false should result in false", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: pod.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureFlagDisableAffinityAssistantKey: "false", }, @@ -158,7 +160,7 @@ func TestDisableAffinityAssistant(t *testing.T) { }, { description: "Setting disable-affinity-assistant to true should result in true", configMap: &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: pod.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, Data: map[string]string{ featureFlagDisableAffinityAssistantKey: "true", }, @@ -175,7 +177,9 @@ func TestDisableAffinityAssistant(t *testing.T) { Logger: zap.NewExample().Sugar(), }, } - if result := c.isAffinityAssistantDisabled(); result != tc.expected { + store := config.NewStore(logtesting.TestLogger(t)) + store.OnConfigChanged(tc.configMap) + if result := c.isAffinityAssistantDisabled(store.ToContext(context.Background())); result != tc.expected { t.Errorf("Expected %t Received %t", tc.expected, result) } }) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index 6a7d9fb3202..5b01c72abe4 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -502,7 +502,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err } } - if !c.isAffinityAssistantDisabled() { + if !c.isAffinityAssistantDisabled(ctx) { // create Affinity Assistant (StatefulSet) so that taskRun pods that share workspace PVC achieve Node Affinity if err = c.createAffinityAssistants(pr.Spec.Workspaces, pr, pr.Namespace); err != nil { c.Logger.Errorf("Failed to create affinity assistant StatefulSet for PipelineRun %s: %v", pr.Name, err) @@ -540,7 +540,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1beta1.PipelineRun) err continue } if rprt.ResolvedConditionChecks == nil || rprt.ResolvedConditionChecks.IsSuccess() { - rprt.TaskRun, err = c.createTaskRun(rprt, pr, as.StorageBasePath(pr)) + rprt.TaskRun, err = c.createTaskRun(ctx, rprt, pr, as.StorageBasePath(pr)) if err != nil { c.Recorder.Eventf(pr, corev1.EventTypeWarning, "TaskRunCreationFailed", "Failed to create TaskRun %q: %v", rprt.TaskRunName, err) return fmt.Errorf("error creating TaskRun called %s for PipelineTask %s from PipelineRun %s: %w", rprt.TaskRunName, rprt.PipelineTask.Name, pr.Name, err) @@ -659,7 +659,7 @@ func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1beta1.PipelineRun) error return nil } -func (c *Reconciler) createTaskRun(rprt *resources.ResolvedPipelineRunTask, pr *v1beta1.PipelineRun, storageBasePath string) (*v1beta1.TaskRun, error) { +func (c *Reconciler) createTaskRun(ctx context.Context, rprt *resources.ResolvedPipelineRunTask, pr *v1beta1.PipelineRun, storageBasePath string) (*v1beta1.TaskRun, error) { tr, _ := c.taskRunLister.TaskRuns(pr.Namespace).Get(rprt.TaskRunName) if tr != nil { //is a retry @@ -714,7 +714,7 @@ func (c *Reconciler) createTaskRun(rprt *resources.ResolvedPipelineRunTask, pr * } } - if !c.isAffinityAssistantDisabled() && pipelinePVCWorkspaceName != "" { + if !c.isAffinityAssistantDisabled(ctx) && pipelinePVCWorkspaceName != "" { tr.Annotations[workspace.AnnotationAffinityAssistantName] = getAffinityAssistantName(pipelinePVCWorkspaceName, pr.GetOwnerReference()) } diff --git a/pkg/reconciler/taskrun/controller.go b/pkg/reconciler/taskrun/controller.go index a5cd47f0682..7c9bf6caf4e 100644 --- a/pkg/reconciler/taskrun/controller.go +++ b/pkg/reconciler/taskrun/controller.go @@ -20,6 +20,7 @@ import ( "context" "time" + "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/apis/pipeline" "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" pipelineclient "github.com/tektoncd/pipeline/pkg/client/injection/client" @@ -103,6 +104,8 @@ func NewController(namespace string, images pipeline.Images) func(context.Contex Handler: controller.HandleAll(impl.EnqueueControllerOf), }) + c.configStore = config.NewStore(c.Logger.Named("config-store")) + c.configStore.WatchConfigs(cmw) return impl } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index 888ff21da0c..d6a499fa275 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -49,6 +49,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/cache" "knative.dev/pkg/apis" + "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/tracker" ) @@ -58,6 +59,11 @@ const ( taskRunAgentName = "taskrun-controller" ) +type configStore interface { + ToContext(ctx context.Context) context.Context + WatchConfigs(w configmap.Watcher) +} + // Reconciler implements controller.Reconciler for Configuration resources. type Reconciler struct { *reconciler.Base @@ -73,6 +79,7 @@ type Reconciler struct { timeoutHandler *reconciler.TimeoutSet metrics *Recorder pvcHandler volumeclaim.PvcHandler + configStore configStore } // Check that our Reconciler implements controller.Reconciler @@ -90,6 +97,8 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { return nil } + ctx = c.configStore.ToContext(ctx) + // Get the Task Run resource with this namespace/name original, err := c.taskRunLister.TaskRuns(namespace).Get(name) if k8serrors.IsNotFound(err) { @@ -376,7 +385,7 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, tr.Spec.Workspaces = taskRunWorkspaces } - pod, err = c.createPod(tr, rtr) + pod, err = c.createPod(ctx, tr, rtr) if err != nil { c.handlePodCreationError(tr, err) return nil @@ -544,7 +553,7 @@ func (c *Reconciler) failTaskRun(tr *v1beta1.TaskRun, reason, message string) er // createPod creates a Pod based on the Task's configuration, with pvcName as a volumeMount // TODO(dibyom): Refactor resource setup/substitution logic to its own function in the resources package -func (c *Reconciler) createPod(tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources) (*corev1.Pod, error) { +func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskResources) (*corev1.Pod, error) { ts := rtr.TaskSpec.DeepCopy() inputResources, err := resourceImplBinding(rtr.Inputs, c.Images) if err != nil { @@ -600,12 +609,12 @@ func (c *Reconciler) createPod(tr *v1beta1.TaskRun, rtr *resources.ResolvedTaskR } // Check if the HOME env var of every Step should be set to /tekton/home. - shouldOverrideHomeEnv := podconvert.ShouldOverrideHomeEnv(c.KubeClientSet) + shouldOverrideHomeEnv := podconvert.ShouldOverrideHomeEnv(ctx) // Apply creds-init path substitutions. ts = resources.ApplyCredentialsPath(ts, podconvert.CredentialsPath(shouldOverrideHomeEnv)) - pod, err := podconvert.MakePod(c.Images, tr, *ts, c.KubeClientSet, c.entrypointCache, shouldOverrideHomeEnv) + pod, err := podconvert.MakePod(ctx, c.Images, tr, *ts, c.KubeClientSet, c.entrypointCache, shouldOverrideHomeEnv) if err != nil { return nil, fmt.Errorf("translating TaskSpec to Pod: %w", err) } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index b90b449bd58..6319f8389c3 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -209,6 +209,12 @@ var ( }, }, } + credsVolume = corev1.Volume{ + Name: "tekton-creds-init-home", + VolumeSource: corev1.VolumeSource{ + EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}, + }, + } getMkdirResourceContainer = func(name, dir, suffix string, ops ...tb.ContainerOp) tb.PodSpecOp { actualOps := []tb.ContainerOp{ @@ -255,6 +261,30 @@ func getRunName(tr *v1beta1.TaskRun) string { return strings.Join([]string{tr.Namespace, tr.Name}, "/") } +func ensureConfigurationConfigMapsExist(d *test.Data) { + var defaultsExists, featureFlagsExists bool + for _, cm := range d.ConfigMaps { + if cm.Name == config.GetDefaultsConfigName() { + defaultsExists = true + } + if cm.Name == config.GetFeatureFlagsConfigName() { + featureFlagsExists = true + } + } + if !defaultsExists { + d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetDefaultsConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{}, + }) + } + if !featureFlagsExists { + d.ConfigMaps = append(d.ConfigMaps, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{}, + }) + } +} + // getTaskRunController returns an instance of the TaskRun controller/reconciler that has been seeded with // d, where d represents the state of the system (existing resources) needed for the test. func getTaskRunController(t *testing.T, d test.Data) (test.Assets, func()) { @@ -265,13 +295,25 @@ func getTaskRunController(t *testing.T, d test.Data) (test.Assets, func()) { SendSuccessfully: true, } ctx = cloudevent.WithClient(ctx, &cloudEventClientBehaviour) + ensureConfigurationConfigMapsExist(&d) c, informers := test.SeedTestData(t, ctx, d) configMapWatcher := configmap.NewInformedWatcher(c.Kube, system.GetNamespace()) + + controller := NewController(namespace, images)(ctx, configMapWatcher) + + stopCh := make(chan struct{}) + if err := configMapWatcher.Start(stopCh); err != nil { + t.Fatalf("error starting configmap watcher: %v", err) + } + return test.Assets{ - Controller: NewController(namespace, images)(ctx, configMapWatcher), - Clients: c, - Informers: informers, - }, cancel + Controller: controller, + Clients: c, + Informers: informers, + }, func() { + close(stopCh) + cancel() + } } func checkEvents(fr *record.FakeRecorder, testName string, wantEvents []string) error { @@ -314,20 +356,21 @@ func TestReconcile_ExplicitDefaultSA(t *testing.T) { tb.TaskRunServiceAccountName("test-sa"), )) taskruns := []*v1beta1.TaskRun{taskRunSuccess, taskRunWithSaSuccess} + defaultSAName := "pipelines" d := test.Data{ TaskRuns: taskruns, Tasks: []*v1beta1.Task{simpleTask, saTask}, - } - - defaultSAName := "pipelines" - defaultCfg := &config.Config{ - Defaults: &config.Defaults{ - DefaultServiceAccount: defaultSAName, - DefaultTimeoutMinutes: 60, - DefaultManagedByLabelValue: "tekton-pipelines", + ConfigMaps: []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetDefaultsConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{ + "default-service-account": defaultSAName, + "default-timeout-minutes": "60", + "default-managed-by-label-value": "tekton-pipelines", + }, + }, }, } - for _, tc := range []struct { name string taskRun *v1beta1.TaskRun @@ -432,8 +475,7 @@ func TestReconcile_ExplicitDefaultSA(t *testing.T) { t.Fatal(err) } - ctx := config.ToContext(context.Background(), defaultCfg) - if err := c.Reconciler.Reconcile(ctx, getRunName(tc.taskRun)); err != nil { + if err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.taskRun)); err != nil { t.Errorf("expected no error. Got error %v", err) } if len(clients.Kube.Actions()) == 0 { @@ -480,6 +522,186 @@ func TestReconcile_ExplicitDefaultSA(t *testing.T) { } } +// TestReconcile_FeatureFlags tests taskruns with and without feature flags set +// to ensure the 'feature-flags' config map can be used to disable the +// corresponding behavior. +func TestReconcile_FeatureFlags(t *testing.T) { + taskWithEnvVar := tb.Task("test-task-with-env-var", + tb.TaskSpec(tb.Step("foo", + tb.StepName("simple-step"), tb.StepCommand("/mycmd"), tb.StepEnvVar("foo", "bar"), + )), + tb.TaskNamespace("foo"), + ) + taskRunWithDisableHomeEnv := tb.TaskRun("test-taskrun-run-home-env", + tb.TaskRunNamespace("foo"), + tb.TaskRunSpec(tb.TaskRunTaskRef(taskWithEnvVar.Name)), + ) + taskRunWithDisableWorkingDirOverwrite := tb.TaskRun("test-taskrun-run-working-dir", + tb.TaskRunNamespace("foo"), + tb.TaskRunSpec(tb.TaskRunTaskRef(simpleTask.Name)), + ) + d := test.Data{ + TaskRuns: []*v1beta1.TaskRun{taskRunWithDisableHomeEnv, taskRunWithDisableWorkingDirOverwrite}, + Tasks: []*v1beta1.Task{simpleTask, taskWithEnvVar}, + } + for _, tc := range []struct { + name string + taskRun *v1beta1.TaskRun + featureFlag string + wantPod *corev1.Pod + }{{ + name: "disable-home-env-overwrite", + taskRun: taskRunWithDisableHomeEnv, + featureFlag: "disable-home-env-overwrite", + wantPod: tb.Pod("test-taskrun-run-home-env-pod-abcde", + tb.PodNamespace("foo"), + tb.PodAnnotation(podconvert.ReleaseAnnotation, podconvert.ReleaseAnnotationValue), + tb.PodLabel(taskNameLabelKey, "test-task-with-env-var"), + tb.PodLabel(taskRunNameLabelKey, "test-taskrun-run-home-env"), + tb.PodLabel("app.kubernetes.io/managed-by", "tekton-pipelines"), + tb.PodOwnerReference("TaskRun", "test-taskrun-run-home-env", + tb.OwnerReferenceAPIVersion(currentAPIVersion)), + tb.PodSpec( + tb.PodVolumes(workspaceVolume, homeVolume, resultsVolume, credsVolume, toolsVolume, downwardVolume), + tb.PodRestartPolicy(corev1.RestartPolicyNever), + getPlaceToolsInitContainer(), + tb.PodContainer("step-simple-step", "foo", + tb.Command(entrypointLocation), + tb.Args("-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/tools/0", + "-termination_path", + "/tekton/termination", + "-entrypoint", + "/mycmd", + "--", + ), + tb.WorkingDir(workspaceDir), + tb.EnvVar("foo", "bar"), + tb.VolumeMount("tekton-internal-tools", "/tekton/tools"), + tb.VolumeMount("tekton-internal-downward", "/tekton/downward"), + tb.VolumeMount("tekton-internal-workspace", workspaceDir), + tb.VolumeMount("tekton-internal-home", "/tekton/home"), + tb.VolumeMount("tekton-internal-results", "/tekton/results"), + tb.VolumeMount("tekton-creds-init-home", "/tekton/creds"), + tb.TerminationMessagePath("/tekton/termination"), + ), + ), + ), + }, { + name: "disable-working-dir-overwrite", + taskRun: taskRunWithDisableWorkingDirOverwrite, + featureFlag: "disable-working-directory-overwrite", + wantPod: tb.Pod("test-taskrun-run-working-dir-pod-abcde", + tb.PodNamespace("foo"), + tb.PodAnnotation(podconvert.ReleaseAnnotation, podconvert.ReleaseAnnotationValue), + tb.PodLabel(taskNameLabelKey, "test-task"), + tb.PodLabel(taskRunNameLabelKey, "test-taskrun-run-working-dir"), + tb.PodLabel("app.kubernetes.io/managed-by", "tekton-pipelines"), + tb.PodOwnerReference("TaskRun", "test-taskrun-run-working-dir", + tb.OwnerReferenceAPIVersion(currentAPIVersion)), + tb.PodSpec( + tb.PodVolumes(workspaceVolume, homeVolume, resultsVolume, toolsVolume, downwardVolume), + tb.PodRestartPolicy(corev1.RestartPolicyNever), + getPlaceToolsInitContainer(), + tb.PodContainer("step-simple-step", "foo", + tb.Command(entrypointLocation), + tb.Args("-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/tools/0", + "-termination_path", + "/tekton/termination", + "-entrypoint", + "/mycmd", + "--", + ), + tb.EnvVar("HOME", "/tekton/home"), + tb.VolumeMount("tekton-internal-tools", "/tekton/tools"), + tb.VolumeMount("tekton-internal-downward", "/tekton/downward"), + tb.VolumeMount("tekton-internal-workspace", workspaceDir), + tb.VolumeMount("tekton-internal-home", "/tekton/home"), + tb.VolumeMount("tekton-internal-results", "/tekton/results"), + tb.TerminationMessagePath("/tekton/termination"), + ), + ), + ), + }} { + t.Run(tc.name, func(t *testing.T) { + names.TestingSeed() + d.ConfigMaps = []*corev1.ConfigMap{ + { + ObjectMeta: metav1.ObjectMeta{Name: config.GetFeatureFlagsConfigName(), Namespace: system.GetNamespace()}, + Data: map[string]string{ + tc.featureFlag: "true", + }, + }, + } + testAssets, cancel := getTaskRunController(t, d) + defer cancel() + c := testAssets.Controller + clients := testAssets.Clients + saName := tc.taskRun.Spec.ServiceAccountName + if saName == "" { + saName = "default" + } + if _, err := clients.Kube.CoreV1().ServiceAccounts(tc.taskRun.Namespace).Create(&corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: saName, + Namespace: tc.taskRun.Namespace, + }, + }); err != nil { + t.Fatal(err) + } + if err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.taskRun)); err != nil { + t.Errorf("expected no error. Got error %v", err) + } + if len(clients.Kube.Actions()) == 0 { + t.Errorf("Expected actions to be logged in the kubeclient, got none") + } + + namespace, name, err := cache.SplitMetaNamespaceKey(tc.taskRun.Name) + if err != nil { + t.Errorf("Invalid resource key: %v", err) + } + + tr, err := clients.Pipeline.TektonV1beta1().TaskRuns(namespace).Get(name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting updated taskrun: %v", err) + } + condition := tr.Status.GetCondition(apis.ConditionSucceeded) + if condition == nil || condition.Status != corev1.ConditionUnknown { + t.Errorf("Expected invalid TaskRun to have in progress status, but had %v", condition) + } + if condition != nil && condition.Reason != podconvert.ReasonRunning { + t.Errorf("Expected reason %q but was %s", podconvert.ReasonRunning, condition.Reason) + } + + if tr.Status.PodName == "" { + t.Fatalf("Reconcile didn't set pod name") + } + + pod, err := clients.Kube.CoreV1().Pods(tr.Namespace).Get(tr.Status.PodName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to fetch build pod: %v", err) + } + + if d := cmp.Diff(tc.wantPod.ObjectMeta, pod.ObjectMeta, ignoreRandomPodNameSuffix); d != "" { + t.Errorf("Pod metadata doesn't match %s", diff.PrintWantGot(d)) + } + + if d := cmp.Diff(tc.wantPod.Spec, pod.Spec, resourceQuantityCmp); d != "" { + t.Errorf("Pod spec doesn't match, %s", diff.PrintWantGot(d)) + } + if len(clients.Kube.Actions()) == 0 { + t.Fatalf("Expected actions to be logged in the kubeclient, got none") + } + }) + } +} func TestReconcile(t *testing.T) { taskRunSuccess := tb.TaskRun("test-taskrun-run-success", tb.TaskRunNamespace("foo"), tb.TaskRunSpec( tb.TaskRunTaskRef(simpleTask.Name, tb.TaskRefAPIVersion("a1")), @@ -1306,8 +1528,8 @@ func TestReconcileInvalidTaskRuns(t *testing.T) { // Check actions and events actions := clients.Kube.Actions() - if len(actions) != 1 || actions[0].Matches("namespaces", "list") { - t.Errorf("expected one action (list namespaces) created by the reconciler, got %d. Actions: %#v", len(actions), actions) + if len(actions) != 3 || actions[0].Matches("namespaces", "list") { + t.Errorf("expected 3 actions (first: list namespaces) created by the reconciler, got %d. Actions: %#v", len(actions), actions) } err = checkEvents(fr, tc.name, tc.wantEvents) @@ -1377,7 +1599,7 @@ func makePod(taskRun *v1beta1.TaskRun, task *v1beta1.Task) (*corev1.Pod, error) return nil, err } - return podconvert.MakePod(images, taskRun, task.Spec, kubeclient, entrypointCache, true) + return podconvert.MakePod(context.Background(), images, taskRun, task.Spec, kubeclient, entrypointCache, true) } func TestReconcilePodUpdateStatus(t *testing.T) { diff --git a/test/controller.go b/test/controller.go index 02b105649d9..910686482ec 100644 --- a/test/controller.go +++ b/test/controller.go @@ -41,6 +41,7 @@ import ( coreinformers "k8s.io/client-go/informers/core/v1" fakekubeclientset "k8s.io/client-go/kubernetes/fake" fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake" + fakeconfigmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake" fakepodinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake" "knative.dev/pkg/controller" ) @@ -57,6 +58,7 @@ type Data struct { Conditions []*v1alpha1.Condition Pods []*corev1.Pod Namespaces []*corev1.Namespace + ConfigMaps []*corev1.ConfigMap } // Clients holds references to clients which are useful for reconciler tests. @@ -76,6 +78,7 @@ type Informers struct { PipelineResource resourceinformersv1alpha1.PipelineResourceInformer Condition informersv1alpha1.ConditionInformer Pod coreinformers.PodInformer + ConfigMap coreinformers.ConfigMapInformer } // Assets holds references to the controller, logs, clients, and informers. @@ -104,6 +107,7 @@ func SeedTestData(t *testing.T, ctx context.Context, d Data) (Clients, Informers PipelineResource: fakeresourceinformer.Get(ctx), Condition: fakeconditioninformer.Get(ctx), Pod: fakepodinformer.Get(ctx), + ConfigMap: fakeconfigmapinformer.Get(ctx), } for _, pr := range d.PipelineRuns { @@ -184,6 +188,14 @@ func SeedTestData(t *testing.T, ctx context.Context, d Data) (Clients, Informers t.Fatal(err) } } + for _, cm := range d.ConfigMaps { + if err := i.ConfigMap.Informer().GetIndexer().Add(cm); err != nil { + t.Fatal(err) + } + if _, err := c.Kube.CoreV1().ConfigMaps(cm.Namespace).Create(cm); err != nil { + t.Fatal(err) + } + } c.Pipeline.ClearActions() c.Kube.ClearActions() return c, i diff --git a/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/configmap.go b/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/configmap.go new file mode 100644 index 00000000000..17c854540c7 --- /dev/null +++ b/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/configmap.go @@ -0,0 +1,52 @@ +/* +Copyright 2020 The Knative 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. +*/ + +// Code generated by injection-gen. DO NOT EDIT. + +package configmap + +import ( + context "context" + + v1 "k8s.io/client-go/informers/core/v1" + factory "knative.dev/pkg/client/injection/kube/informers/factory" + controller "knative.dev/pkg/controller" + injection "knative.dev/pkg/injection" + logging "knative.dev/pkg/logging" +) + +func init() { + injection.Default.RegisterInformer(withInformer) +} + +// Key is used for associating the Informer inside the context.Context. +type Key struct{} + +func withInformer(ctx context.Context) (context.Context, controller.Informer) { + f := factory.Get(ctx) + inf := f.Core().V1().ConfigMaps() + return context.WithValue(ctx, Key{}, inf), inf.Informer() +} + +// Get extracts the typed informer from the context. +func Get(ctx context.Context) v1.ConfigMapInformer { + untyped := ctx.Value(Key{}) + if untyped == nil { + logging.FromContext(ctx).Panic( + "Unable to fetch k8s.io/client-go/informers/core/v1.ConfigMapInformer from context.") + } + return untyped.(v1.ConfigMapInformer) +} diff --git a/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake/fake.go b/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake/fake.go new file mode 100644 index 00000000000..73a324fa77d --- /dev/null +++ b/vendor/knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake/fake.go @@ -0,0 +1,40 @@ +/* +Copyright 2020 The Knative 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. +*/ + +// Code generated by injection-gen. DO NOT EDIT. + +package fake + +import ( + context "context" + + configmap "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap" + fake "knative.dev/pkg/client/injection/kube/informers/factory/fake" + controller "knative.dev/pkg/controller" + injection "knative.dev/pkg/injection" +) + +var Get = configmap.Get + +func init() { + injection.Fake.RegisterInformer(withInformer) +} + +func withInformer(ctx context.Context) (context.Context, controller.Informer) { + f := fake.Get(ctx) + inf := f.Core().V1().ConfigMaps() + return context.WithValue(ctx, configmap.Key{}, inf), inf.Informer() +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 86fd9925138..4ccc74438f6 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -941,6 +941,8 @@ knative.dev/pkg/client/injection/kube/client knative.dev/pkg/client/injection/kube/client/fake knative.dev/pkg/client/injection/kube/informers/admissionregistration/v1beta1/mutatingwebhookconfiguration knative.dev/pkg/client/injection/kube/informers/admissionregistration/v1beta1/validatingwebhookconfiguration +knative.dev/pkg/client/injection/kube/informers/core/v1/configmap +knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake knative.dev/pkg/client/injection/kube/informers/core/v1/pod knative.dev/pkg/client/injection/kube/informers/core/v1/pod/fake knative.dev/pkg/client/injection/kube/informers/factory