Only mount Downward API when necessary

Instead of always mounting a Downward API volume for reading the "ready"
annotation on task Pods, we only create it if the pod will not be
"immediately ready" (i.e. if it has sidecars).

In addition, a new feature flag is added: "await-sidecar-readiness".
Setting this to `false` will mean that pods will always be considered
"immediately ready", so the Downward API volume will never be mounted.

config/config-feature-flags.yaml

@@ -44,6 +44,14 @@ data:
   # See https://github.com/tektoncd/pipeline/issues/2791 for more
   # info.
   disable-creds-init: "false"
+  # Setting this flag to "false" will stop Tekton from waiting for a
+  # TaskRun's sidecar containers to be running before starting the first
+  # step. This will allow Tasks to be run in environments that don't
+  # support the DownwardAPI volume type, but may lead to unintended
+  # behaviour if sidecars are used.
+  #
+  # See https://github.com/tektoncd/pipeline/issues/4937 for more info.
+  await-sidecar-readiness: "true"
   # This option should be set to false when Pipelines is running in a
   # cluster that does not use injected sidecars such as Istio. Setting
   # it to false should decrease the time it takes for a TaskRun to start

docs/install.md

@@ -355,11 +355,18 @@ To customize the behavior of the Pipelines Controller, modify the ConfigMap `fea
   node in the cluster must have an appropriate label matching `topologyKey`. If some or all nodes
   are missing the specified `topologyKey` label, it can lead to unintended behavior.
 
-- `running-in-environment-with-injected-sidecars`: set this flag to `"true"` to allow the
-Tekton controller to set the `tekton.dev/ready` annotation at pod creation time for
-TaskRuns with no Sidecars specified. Enabling this option should decrease the time it takes for a TaskRun to
-start running. However, for clusters that use injected sidecars e.g. istio
-enabling this option can lead to unexpected behavior.
+- `await-sidecar-readiness`: set this flag to `"false"` to allow the Tekton controller to start a
+TasksRun's first step immediately without waiting for sidecar containers to be running first. Using
+this option should decrease the time it takes for a TaskRun to start running, and will allow TaskRun
+pods to be scheduled in environments that don't support [Downward API](https://kubernetes.io/docs/tasks/inject-data-application/downward-api-volume-expose-pod-information/)
+volumes (e.g. some virtual kubelet implementations). However, this may lead to unexpected behaviour
+with Tasks that use sidecars, or in clusters that use injected sidecars (e.g. Istio). Setting this flag
+to `"false"` will mean the `running-in-environment-with-injected-sidecars` flag has no effect.
+
+- `running-in-environment-with-injected-sidecars`: set this flag to `"false"` to allow the
+Tekton controller to start a TasksRun's first step immediately if it has no Sidecars specified.
+Using this option should decrease the time it takes for a TaskRun to start running.
+However, for clusters that use injected sidecars (e.g. Istio) this can lead to unexpected behavior.
 
 - `require-git-ssh-secret-known-hosts`: set this flag to `"true"` to require that
 Git SSH Secrets include a `known_hosts` field. This ensures that a git remote server's

pkg/apis/config/feature_flags.go

@@ -45,6 +45,8 @@ const (
 	DefaultDisableCredsInit = false
 	// DefaultRunningInEnvWithInjectedSidecars is the default value for "running-in-environment-with-injected-sidecars".
 	DefaultRunningInEnvWithInjectedSidecars = true
+	// DefaultAwaitSidecarReadiness is the default value for "await-sidecar-readiness".
+	DefaultAwaitSidecarReadiness = true
 	// DefaultRequireGitSSHSecretKnownHosts is the default value for "require-git-ssh-secret-known-hosts".
 	DefaultRequireGitSSHSecretKnownHosts = false
 	// DefaultEnableTektonOciBundles is the default value for "enable-tekton-oci-bundles".
@@ -61,6 +63,7 @@ const (
 	disableAffinityAssistantKey         = "disable-affinity-assistant"
 	disableCredsInitKey                 = "disable-creds-init"
 	runningInEnvWithInjectedSidecarsKey = "running-in-environment-with-injected-sidecars"
+	awaitSidecarReadinessKey            = "await-sidecar-readiness"
 	requireGitSSHSecretKnownHostsKey    = "require-git-ssh-secret-known-hosts" // nolint: gosec
 	enableTektonOCIBundles              = "enable-tekton-oci-bundles"
 	enableCustomTasks                   = "enable-custom-tasks"
@@ -81,6 +84,7 @@ type FeatureFlags struct {
 	ScopeWhenExpressionsToTask       bool
 	EnableAPIFields                  string
 	SendCloudEventsForRuns           bool
+	AwaitSidecarReadiness            bool
 	EmbeddedStatus                   string
 }
 
@@ -118,6 +122,9 @@ func NewFeatureFlagsFromMap(cfgMap map[string]string) (*FeatureFlags, error) {
 	if err := setFeature(runningInEnvWithInjectedSidecarsKey, DefaultRunningInEnvWithInjectedSidecars, &tc.RunningInEnvWithInjectedSidecars); err != nil {
 		return nil, err
 	}
+	if err := setFeature(awaitSidecarReadinessKey, DefaultAwaitSidecarReadiness, &tc.AwaitSidecarReadiness); err != nil {
+		return nil, err
+	}
 	if err := setFeature(requireGitSSHSecretKnownHostsKey, DefaultRequireGitSSHSecretKnownHosts, &tc.RequireGitSSHSecretKnownHosts); err != nil {
 		return nil, err
 	}

pkg/apis/config/feature_flags_test.go

@@ -35,16 +35,25 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
 	testCases := []testCase{
 		{
 			expectedConfig: &config.FeatureFlags{
-				RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
-				EnableAPIFields:                  "stable",
-				EmbeddedStatus:                   config.DefaultEmbeddedStatus,
+				DisableAffinityAssistant:         false,
+				RunningInEnvWithInjectedSidecars: true,
+				RequireGitSSHSecretKnownHosts:    false,
+
+				DisableCredsInit:       config.DefaultDisableCredsInit,
+				AwaitSidecarReadiness:  config.DefaultAwaitSidecarReadiness,
+				EnableTektonOCIBundles: config.DefaultEnableTektonOciBundles,
+				EnableCustomTasks:      config.DefaultEnableCustomTasks,
+				EnableAPIFields:        config.DefaultEnableAPIFields,
+				SendCloudEventsForRuns: config.DefaultSendCloudEventsForRuns,
+				EmbeddedStatus:         config.DefaultEmbeddedStatus,
 			},
 			fileName: config.GetFeatureFlagsConfigName(),
 		},
 		{
 			expectedConfig: &config.FeatureFlags{
 				DisableAffinityAssistant:         true,
 				RunningInEnvWithInjectedSidecars: false,
+				AwaitSidecarReadiness:            false,
 				RequireGitSSHSecretKnownHosts:    true,
 				EnableTektonOCIBundles:           true,
 				EnableCustomTasks:                true,
@@ -62,7 +71,12 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
 				EnableTektonOCIBundles: true,
 				EnableCustomTasks:      true,
 
+				DisableAffinityAssistant:         config.DefaultDisableAffinityAssistant,
+				DisableCredsInit:                 config.DefaultDisableCredsInit,
 				RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
+				AwaitSidecarReadiness:            config.DefaultAwaitSidecarReadiness,
+				RequireGitSSHSecretKnownHosts:    config.DefaultRequireGitSSHSecretKnownHosts,
+				SendCloudEventsForRuns:           config.DefaultSendCloudEventsForRuns,
 				EmbeddedStatus:                   config.DefaultEmbeddedStatus,
 			},
 			fileName: "feature-flags-enable-api-fields-overrides-bundles-and-custom-tasks",
@@ -73,7 +87,12 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
 				EnableTektonOCIBundles: true,
 				EnableCustomTasks:      true,
 
+				DisableAffinityAssistant:         config.DefaultDisableAffinityAssistant,
+				DisableCredsInit:                 config.DefaultDisableCredsInit,
 				RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
+				AwaitSidecarReadiness:            config.DefaultAwaitSidecarReadiness,
+				RequireGitSSHSecretKnownHosts:    config.DefaultRequireGitSSHSecretKnownHosts,
+				SendCloudEventsForRuns:           config.DefaultSendCloudEventsForRuns,
 				EmbeddedStatus:                   config.DefaultEmbeddedStatus,
 			},
 			fileName: "feature-flags-bundles-and-custom-tasks",
@@ -92,8 +111,15 @@ func TestNewFeatureFlagsFromConfigMap(t *testing.T) {
 func TestNewFeatureFlagsFromEmptyConfigMap(t *testing.T) {
 	FeatureFlagsConfigEmptyName := "feature-flags-empty"
 	expectedConfig := &config.FeatureFlags{
-		RunningInEnvWithInjectedSidecars: true,
-		EnableAPIFields:                  "stable",
+		DisableAffinityAssistant:         config.DefaultDisableAffinityAssistant,
+		DisableCredsInit:                 config.DefaultDisableCredsInit,
+		RunningInEnvWithInjectedSidecars: config.DefaultRunningInEnvWithInjectedSidecars,
+		AwaitSidecarReadiness:            config.DefaultAwaitSidecarReadiness,
+		RequireGitSSHSecretKnownHosts:    config.DefaultRequireGitSSHSecretKnownHosts,
+		EnableTektonOCIBundles:           config.DefaultEnableTektonOciBundles,
+		EnableCustomTasks:                config.DefaultEnableCustomTasks,
+		EnableAPIFields:                  config.DefaultEnableAPIFields,
+		SendCloudEventsForRuns:           config.DefaultSendCloudEventsForRuns,
 		EmbeddedStatus:                   config.DefaultEmbeddedStatus,
 	}
 	verifyConfigFileWithExpectedFeatureFlagsConfig(t, FeatureFlagsConfigEmptyName, expectedConfig)

pkg/apis/config/testdata/feature-flags-all-flags-set.yaml

@@ -20,6 +20,7 @@ metadata:
 data:
   disable-affinity-assistant: "true"
   running-in-environment-with-injected-sidecars: "false"
+  await-sidecar-readiness: "false"
   require-git-ssh-secret-known-hosts: "true"
   enable-tekton-oci-bundles: "true"
   enable-custom-tasks: "true"

pkg/pod/entrypoint.go

@@ -108,34 +108,31 @@ var (
 // command, we must have fetched the image's ENTRYPOINT before calling this
 // method, using entrypoint_lookup.go.
 // Additionally, Step timeouts are added as entrypoint flag.
-func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Container, taskSpec *v1beta1.TaskSpec, breakpointConfig *v1beta1.TaskRunDebug) ([]corev1.Container, error) {
+func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Container, taskSpec *v1beta1.TaskSpec, breakpointConfig *v1beta1.TaskRunDebug, waitForReadyAnnotation bool) ([]corev1.Container, error) {
 	if len(steps) == 0 {
 		return nil, errors.New("No steps specified")
 	}
 
 	for i, s := range steps {
-		var argsForEntrypoint []string
+		var argsForEntrypoint = []string{}
 		idx := strconv.Itoa(i)
-		switch i {
-		case 0:
-			argsForEntrypoint = []string{
-				// First step waits for the Downward volume file.
-				"-wait_file", filepath.Join(downwardMountPoint, downwardMountReadyFile),
-				"-wait_file_content", // Wait for file contents, not just an empty file.
-				// Start next step.
-				"-post_file", filepath.Join(runDir, idx, "out"),
-				"-termination_path", terminationPath,
-				"-step_metadata_dir", filepath.Join(runDir, idx, "status"),
-			}
-		default:
-			// All other steps wait for previous file, write next file.
-			argsForEntrypoint = []string{
-				"-wait_file", filepath.Join(runDir, strconv.Itoa(i-1), "out"),
-				"-post_file", filepath.Join(runDir, idx, "out"),
-				"-termination_path", terminationPath,
-				"-step_metadata_dir", filepath.Join(runDir, idx, "status"),
+		if i == 0 {
+			if waitForReadyAnnotation {
+				argsForEntrypoint = append(argsForEntrypoint,
+					// First step waits for the Downward volume file.
+					"-wait_file", filepath.Join(downwardMountPoint, downwardMountReadyFile),
+					"-wait_file_content", // Wait for file contents, not just an empty file.
+				)
 			}
+		} else { // Not the first step - wait for previous
+			argsForEntrypoint = append(argsForEntrypoint, "-wait_file", filepath.Join(runDir, strconv.Itoa(i-1), "out"))
 		}
+		argsForEntrypoint = append(argsForEntrypoint,
+			// Start next step.
+			"-post_file", filepath.Join(runDir, idx, "out"),
+			"-termination_path", terminationPath,
+			"-step_metadata_dir", filepath.Join(runDir, idx, "status"),
+		)
 		argsForEntrypoint = append(argsForEntrypoint, commonExtraEntrypointArgs...)
 		if taskSpec != nil {
 			if taskSpec.Steps != nil && len(taskSpec.Steps) >= i+1 {
@@ -173,8 +170,10 @@ func orderContainers(commonExtraEntrypointArgs []string, steps []corev1.Containe
 		steps[i].Args = argsForEntrypoint
 		steps[i].TerminationMessagePath = terminationPath
 	}
-	// Mount the Downward volume into the first step container.
-	steps[0].VolumeMounts = append(steps[0].VolumeMounts, downwardMount)
+	if waitForReadyAnnotation {
+		// Mount the Downward volume into the first step container.
+		steps[0].VolumeMounts = append(steps[0].VolumeMounts, downwardMount)
+	}
 
 	return steps, nil
 }

pkg/pod/entrypoint_test.go

@@ -93,7 +93,53 @@ func TestOrderContainers(t *testing.T) {
 		},
 		TerminationMessagePath: "/tekton/termination",
 	}}
-	got, err := orderContainers([]string{}, steps, nil, nil)
+	got, err := orderContainers([]string{}, steps, nil, nil, true)
+	if err != nil {
+		t.Fatalf("orderContainers: %v", err)
+	}
+	if d := cmp.Diff(want, got); d != "" {
+		t.Errorf("Diff %s", diff.PrintWantGot(d))
+	}
+}
+
+func TestOrderContainersWithNoWait(t *testing.T) {
+	steps := []corev1.Container{{
+		Image:   "step-1",
+		Command: []string{"cmd"},
+		Args:    []string{"arg1", "arg2"},
+	}, {
+		Image:        "step-2",
+		Command:      []string{"cmd1", "cmd2", "cmd3"}, // multiple cmd elements
+		Args:         []string{"arg1", "arg2"},
+		VolumeMounts: []corev1.VolumeMount{volumeMount}, // pre-existing volumeMount
+	}}
+	want := []corev1.Container{{
+		Image:   "step-1",
+		Command: []string{entrypointBinary},
+		Args: []string{
+			"-post_file", "/tekton/run/0/out",
+			"-termination_path", "/tekton/termination",
+			"-step_metadata_dir", "/tekton/run/0/status",
+			"-entrypoint", "cmd", "--",
+			"arg1", "arg2",
+		},
+		TerminationMessagePath: "/tekton/termination",
+	}, {
+		Image:   "step-2",
+		Command: []string{entrypointBinary},
+		Args: []string{
+			"-wait_file", "/tekton/run/0/out",
+			"-post_file", "/tekton/run/1/out",
+			"-termination_path", "/tekton/termination",
+			"-step_metadata_dir", "/tekton/run/1/status",
+			"-entrypoint", "cmd1", "--",
+			"cmd2", "cmd3",
+			"arg1", "arg2",
+		},
+		VolumeMounts:           []corev1.VolumeMount{volumeMount},
+		TerminationMessagePath: "/tekton/termination",
+	}}
+	got, err := orderContainers([]string{}, steps, nil, nil, false)
 	if err != nil {
 		t.Fatalf("orderContainers: %v", err)
 	}
@@ -127,7 +173,7 @@ func TestOrderContainersWithDebugOnFailure(t *testing.T) {
 	taskRunDebugConfig := &v1beta1.TaskRunDebug{
 		Breakpoint: []string{"onFailure"},
 	}
-	got, err := orderContainers([]string{}, steps, nil, taskRunDebugConfig)
+	got, err := orderContainers([]string{}, steps, nil, taskRunDebugConfig, true)
 	if err != nil {
 		t.Fatalf("orderContainers: %v", err)
 	}
@@ -205,7 +251,7 @@ func TestEntryPointResults(t *testing.T) {
 		},
 		TerminationMessagePath: "/tekton/termination",
 	}}
-	got, err := orderContainers([]string{}, steps, &taskSpec, nil)
+	got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
 	if err != nil {
 		t.Fatalf("orderContainers: %v", err)
 	}
@@ -246,7 +292,7 @@ func TestEntryPointResultsSingleStep(t *testing.T) {
 		VolumeMounts:           []corev1.VolumeMount{downwardMount},
 		TerminationMessagePath: "/tekton/termination",
 	}}
-	got, err := orderContainers([]string{}, steps, &taskSpec, nil)
+	got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
 	if err != nil {
 		t.Fatalf("orderContainers: %v", err)
 	}
@@ -283,7 +329,7 @@ func TestEntryPointSingleResultsSingleStep(t *testing.T) {
 		VolumeMounts:           []corev1.VolumeMount{downwardMount},
 		TerminationMessagePath: "/tekton/termination",
 	}}
-	got, err := orderContainers([]string{}, steps, &taskSpec, nil)
+	got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
 	if err != nil {
 		t.Fatalf("orderContainers: %v", err)
 	}
@@ -340,7 +386,7 @@ func TestEntryPointOnError(t *testing.T) {
 		},
 		TerminationMessagePath: "/tekton/termination",
 	}}
-	got, err := orderContainers([]string{}, steps, &taskSpec, nil)
+	got, err := orderContainers([]string{}, steps, &taskSpec, nil, true)
 	if err != nil {
 		t.Fatalf("orderContainers: %v", err)
 	}