add /var/log volume mount only when EnableVarLogCollection is true

docs/runtime-contract.md

@@ -426,10 +426,10 @@ In addition to the filesystems recommended in the OCI, the following filesystems
 [MUST](https://github.com/knative/serving/blob/master/test/conformance/runtime/filesystem_test.go)
 be provided:
 
-| Mount      | Description                                                                                                                                                      |
-| ---------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
-| `/tmp`     | MUST be Read-write.<p>SHOULD be backed by tmpfs if disk load is a concern.                                                                                       |
-| `/var/log` | MUST be a directory with write permissions for logs storage. Implementations MAY permit the creation of additional subdirectories and log rotation and renaming. |
+| Mount      | Description                                                                                                                                                     |
+| ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- |
+| `/tmp`     | MUST be Read-write.<p>SHOULD be backed by tmpfs if disk load is a concern.                                                                                      |
+| `/var/log` | MAY be a directory with write permissions for logs storage. Implementations MAY permit the creation of additional subdirectories and log rotation and renaming. |
 
 To enable DNS resolution, the following files might be overwritten at runtime:
 

pkg/reconciler/revision/resources/deploy.go

@@ -95,6 +95,23 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
 
 	podSpec := BuildPodSpec(rev, append(BuildUserContainers(rev), *queueContainer))
 
+	if cfg.Observability.EnableVarLogCollection {
+		podSpec.Volumes = append(podSpec.Volumes, varLogVolume)
+
+		for i, container := range podSpec.Containers {
+			if container.Name == QueueContainerName {
+				continue
+			}
+
+			varLogMount := varLogVolumeMount.DeepCopy()
+			varLogMount.SubPathExpr += container.Name
+			container.VolumeMounts = append(container.VolumeMounts, *varLogMount)
+			container.Env = append(container.Env, buildVarLogSubpathEnvs()...)
+
+			podSpec.Containers[i] = container
+		}
+	}
+
 	return podSpec, nil
 }
 
@@ -124,13 +141,9 @@ func BuildUserContainers(rev *v1.Revision) []corev1.Container {
 func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Container {
 	// Adding or removing an overwritten corev1.Container field here? Don't forget to
 	// update the fieldmasks / validations in pkg/apis/serving
-	varLogMount := varLogVolumeMount.DeepCopy()
-	varLogMount.SubPathExpr += container.Name
-
-	container.VolumeMounts = append(container.VolumeMounts, *varLogMount)
 	container.Lifecycle = userLifecycle
 	container.Env = append(container.Env, getKnativeEnvVar(rev)...)
-	container.Env = append(container.Env, buildVarLogSubpathEnvs()...)
+
 	// Explicitly disable stdin and tty allocation
 	container.Stdin = false
 	container.TTY = false
@@ -163,7 +176,6 @@ func makeServingContainer(servingContainer corev1.Container, rev *v1.Revision) c
 func BuildPodSpec(rev *v1.Revision, containers []corev1.Container) *corev1.PodSpec {
 	pod := rev.Spec.PodSpec.DeepCopy()
 	pod.Containers = containers
-	pod.Volumes = append([]corev1.Volume{varLogVolume}, rev.Spec.Volumes...)
 	pod.TerminationGracePeriodSeconds = rev.Spec.TimeoutSeconds
 	return pod
 }

pkg/reconciler/revision/resources/deploy_test.go

@@ -53,14 +53,9 @@ var (
 	sidecarContainerName2        = "sidecar-container-2"
 	sidecarIstioInjectAnnotation = "sidecar.istio.io/inject"
 	defaultServingContainer      = &corev1.Container{
-		Name:  servingContainerName,
-		Image: "busybox",
-		Ports: buildContainerPorts(v1.DefaultUserPort),
-		VolumeMounts: []corev1.VolumeMount{{
-			Name:        varLogVolume.Name,
-			MountPath:   "/var/log",
-			SubPathExpr: "$(K_INTERNAL_POD_NAMESPACE)_$(K_INTERNAL_POD_NAME)_" + servingContainerName,
-		}},
+		Name:                     servingContainerName,
+		Image:                    "busybox",
+		Ports:                    buildContainerPorts(v1.DefaultUserPort),
 		Lifecycle:                userLifecycle,
 		TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
 		Stdin:                    false,
@@ -75,12 +70,6 @@ var (
 			Name: "K_CONFIGURATION",
 		}, {
 			Name: "K_SERVICE",
-		}, {
-			Name:      "K_INTERNAL_POD_NAME",
-			ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}},
-		}, {
-			Name:      "K_INTERNAL_POD_NAMESPACE",
-			ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.namespace"}},
 		}},
 	}
 
@@ -183,7 +172,6 @@ var (
 	}
 
 	defaultPodSpec = &corev1.PodSpec{
-		Volumes:                       []corev1.Volume{varLogVolume},
 		TerminationGracePeriodSeconds: refInt64(45),
 	}
 
@@ -231,13 +219,8 @@ var (
 
 func defaultSidecarContainer(containerName string) *corev1.Container {
 	return &corev1.Container{
-		Name:  containerName,
-		Image: "ubuntu",
-		VolumeMounts: []corev1.VolumeMount{{
-			Name:        varLogVolume.Name,
-			MountPath:   "/var/log",
-			SubPathExpr: "$(K_INTERNAL_POD_NAMESPACE)_$(K_INTERNAL_POD_NAME)_" + containerName,
-		}},
+		Name:                     containerName,
+		Image:                    "ubuntu",
 		Lifecycle:                userLifecycle,
 		TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
 		Stdin:                    false,
@@ -249,12 +232,6 @@ func defaultSidecarContainer(containerName string) *corev1.Container {
 			Name: "K_CONFIGURATION",
 		}, {
 			Name: "K_SERVICE",
-		}, {
-			Name:      "K_INTERNAL_POD_NAME",
-			ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}},
-		}, {
-			Name:      "K_INTERNAL_POD_NAMESPACE",
-			ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.namespace"}},
 		}},
 	}
 }
@@ -968,6 +945,69 @@ func TestMakePodSpec(t *testing.T) {
 				p.EnableServiceLinks = ptr.Bool(false)
 			},
 		),
+	}, {
+		name: "var-log collection enabled",
+		oc: metrics.ObservabilityConfig{
+			EnableVarLogCollection: true,
+		},
+		rev: revision("bar", "foo",
+			withContainers([]corev1.Container{{
+				Name:           servingContainerName,
+				Image:          "busybox",
+				ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
+				Ports:          buildContainerPorts(v1.DefaultUserPort),
+			}, {
+				Name:  sidecarContainerName,
+				Image: "ubuntu",
+			}}),
+			WithContainerStatuses([]v1.ContainerStatus{{
+				ImageDigest: "busybox@sha256:deadbeef",
+			}, {
+				ImageDigest: "ubuntu@sha256:deadbeef",
+			}}),
+		),
+		want: podSpec(
+			[]corev1.Container{
+				servingContainer(func(container *corev1.Container) {
+					container.Image = "busybox@sha256:deadbeef"
+					container.VolumeMounts = []corev1.VolumeMount{{
+						Name:        varLogVolume.Name,
+						MountPath:   "/var/log",
+						SubPathExpr: "$(K_INTERNAL_POD_NAMESPACE)_$(K_INTERNAL_POD_NAME)_" + servingContainerName,
+					}}
+					container.Env = append(container.Env,
+						corev1.EnvVar{
+							Name:      "K_INTERNAL_POD_NAME",
+							ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}},
+						},
+						corev1.EnvVar{
+							Name:      "K_INTERNAL_POD_NAMESPACE",
+							ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.namespace"}},
+						})
+				}),
+				sidecarContainer(sidecarContainerName, func(c *corev1.Container) {
+					c.Image = "ubuntu@sha256:deadbeef"
+					c.VolumeMounts = []corev1.VolumeMount{{
+						Name:        varLogVolume.Name,
+						MountPath:   "/var/log",
+						SubPathExpr: "$(K_INTERNAL_POD_NAMESPACE)_$(K_INTERNAL_POD_NAME)_" + sidecarContainerName,
+					}}
+					c.Env = append(c.Env,
+						corev1.EnvVar{
+							Name:      "K_INTERNAL_POD_NAME",
+							ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}},
+						},
+						corev1.EnvVar{
+							Name:      "K_INTERNAL_POD_NAMESPACE",
+							ValueFrom: &corev1.EnvVarSource{FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.namespace"}},
+						})
+				}),
+				queueContainer(
+					withEnvVar("SERVING_READINESS_PROBE", `{"tcpSocket":{"port":8080,"host":"127.0.0.1"}}`),
+				),
+			},
+			withAppendedVolumes(varLogVolume),
+		),
 	}}
 
 	for _, test := range tests {

test/types/runtime.go

@@ -94,10 +94,11 @@ var MustFiles = map[string]FileInfo{
 		IsDir: ptr.Bool(true),
 		Perm:  "rwxrwxrwx",
 	},
-	"/var/log": {
-		IsDir: ptr.Bool(true),
-		Perm:  "rwxrwxrwx",
-	},
+	// TODO(dprotaso) Re-enable this as a potential MAY requirement
+	// "/var/log": {
+	// 	IsDir: ptr.Bool(true),
+	// 	Perm:  "rwxrwxrwx",
+	// },
 }
 
 // ShouldFiles specifies the file paths and expected permissions that SHOULD be set as specified in the runtime contract.