Fix PreStop hook to use HTTPS with system-internal-tls

The PreStop hook was always using HTTP to contact queue-proxy, causing
TLS errors when system-internal-tls was enabled. Now the scheme is
dynamically set based on the TLS configuration.

Author: Fedosin

pkg/reconciler/revision/resources/deploy.go

@@ -97,20 +97,29 @@ var (
 		MountPath: queue.PodInfoDirectory,
 		ReadOnly:  true,
 	}
+)
+
+// makeUserLifecycle creates the lifecycle configuration for user containers.
+// This PreStop hook is actually calling an endpoint on the queue-proxy
+// because of the way PreStop hooks are called by kubelet. We use this
+// to block the user-container from exiting before the queue-proxy is ready
+// to exit so we can guarantee that there are no more requests in flight.
+func makeUserLifecycle(systemInternalTLSEnabled bool) *corev1.Lifecycle {
+	scheme := corev1.URISchemeHTTP
+	if systemInternalTLSEnabled {
+		scheme = corev1.URISchemeHTTPS
+	}
 
-	// This PreStop hook is actually calling an endpoint on the queue-proxy
-	// because of the way PreStop hooks are called by kubelet. We use this
-	// to block the user-container from exiting before the queue-proxy is ready
-	// to exit so we can guarantee that there are no more requests in flight.
-	userLifecycle = &corev1.Lifecycle{
+	return &corev1.Lifecycle{
 		PreStop: &corev1.LifecycleHandler{
 			HTTPGet: &corev1.HTTPGetAction{
-				Port: intstr.FromInt(networking.QueueAdminPort),
-				Path: queue.RequestQueueDrainPath,
+				Port:   intstr.FromInt(networking.QueueAdminPort),
+				Path:   queue.RequestQueueDrainPath,
+				Scheme: scheme,
 			},
 		},
 	}
-)
+}
 
 func addToken(tokenVolume *corev1.Volume, filename string, audience string, expiry *int64) {
 	if filename == "" || audience == "" {
@@ -205,7 +214,7 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
 		extraVolumes = append(extraVolumes, certVolume(networking.ServingCertName))
 	}
 
-	podSpec := BuildPodSpec(rev, append(BuildUserContainers(rev), *queueContainer), cfg)
+	podSpec := BuildPodSpec(rev, append(BuildUserContainers(rev, cfg), *queueContainer), cfg)
 	podSpec.Volumes = append(podSpec.Volumes, extraVolumes...)
 
 	if val := cfg.Deployment.PodRuntimeClassName(rev.ObjectMeta.Labels); podSpec.RuntimeClassName == nil {
@@ -236,14 +245,14 @@ func makePodSpec(rev *v1.Revision, cfg *config.Config) (*corev1.PodSpec, error)
 }
 
 // BuildUserContainers makes an array of containers from the Revision template.
-func BuildUserContainers(rev *v1.Revision) []corev1.Container {
+func BuildUserContainers(rev *v1.Revision, cfg *config.Config) []corev1.Container {
 	containers := make([]corev1.Container, 0, len(rev.Spec.PodSpec.Containers))
 	for i := range rev.Spec.PodSpec.Containers {
 		var container corev1.Container
 		if len(rev.Spec.PodSpec.Containers[i].Ports) != 0 || len(rev.Spec.PodSpec.Containers) == 1 {
-			container = makeServingContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev)
+			container = makeServingContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev, cfg)
 		} else {
-			container = makeContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev)
+			container = makeContainer(*rev.Spec.PodSpec.Containers[i].DeepCopy(), rev, cfg)
 		}
 		// The below logic is safe because the image digests in Status.ContainerStatus will have been resolved
 		// before this method is called. We check for an empty array here because the method can also be
@@ -258,10 +267,11 @@ func BuildUserContainers(rev *v1.Revision) []corev1.Container {
 	return containers
 }
 
-func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Container {
+func makeContainer(container corev1.Container, rev *v1.Revision, cfg *config.Config) corev1.Container {
 	// Adding or removing an overwritten corev1.Container field here? Don't forget to
 	// update the fieldmasks / validations in pkg/apis/serving
-	container.Lifecycle = userLifecycle
+	systemInternalTLSEnabled := cfg != nil && cfg.Network.SystemInternalTLSEnabled()
+	container.Lifecycle = makeUserLifecycle(systemInternalTLSEnabled)
 	container.Env = append(container.Env, getKnativeEnvVar(rev)...)
 
 	// Explicitly disable stdin and tty allocation
@@ -282,13 +292,13 @@ func makeContainer(container corev1.Container, rev *v1.Revision) corev1.Containe
 	return container
 }
 
-func makeServingContainer(servingContainer corev1.Container, rev *v1.Revision) corev1.Container {
+func makeServingContainer(servingContainer corev1.Container, rev *v1.Revision, cfg *config.Config) corev1.Container {
 	userPort := getUserPort(rev)
 	userPortStr := strconv.Itoa(int(userPort))
 	// Replacement is safe as only up to a single port is allowed on the Revision
 	servingContainer.Ports = buildContainerPorts(userPort)
 	servingContainer.Env = append(servingContainer.Env, buildUserPortEnv(userPortStr))
-	container := makeContainer(servingContainer, rev)
+	container := makeContainer(servingContainer, rev, cfg)
 	// If the user provides a liveness probe, we should rewrite in the port on the user-container for them.
 	rewriteUserLivenessProbe(container.LivenessProbe, int(userPort))
 	return container

pkg/reconciler/revision/resources/deploy_test.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/sets"
 
+	netcfg "knative.dev/networking/pkg/config"
 	netheader "knative.dev/networking/pkg/http/header"
 	"knative.dev/pkg/kmeta"
 	"knative.dev/pkg/ptr"
@@ -41,6 +42,7 @@ import (
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
 	"knative.dev/serving/pkg/deployment"
+	"knative.dev/serving/pkg/networking"
 	"knative.dev/serving/pkg/observability"
 	"knative.dev/serving/pkg/queue"
 
@@ -56,7 +58,7 @@ var (
 		Name:                     servingContainerName,
 		Image:                    "busybox",
 		Ports:                    buildContainerPorts(v1.DefaultUserPort),
-		Lifecycle:                userLifecycle,
+		Lifecycle:                makeUserLifecycle(false),
 		TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
 		Stdin:                    false,
 		TTY:                      false,
@@ -253,7 +255,7 @@ func defaultSidecarContainer(containerName string) *corev1.Container {
 	return &corev1.Container{
 		Name:                     containerName,
 		Image:                    "ubuntu",
-		Lifecycle:                userLifecycle,
+		Lifecycle:                makeUserLifecycle(false),
 		TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
 		Stdin:                    false,
 		TTY:                      false,
@@ -1749,6 +1751,112 @@ func TestMakePodSpec(t *testing.T) {
 	}
 }
 
+func TestMakePodSpecWithSystemInternalTLS(t *testing.T) {
+	tests := []struct {
+		name       string
+		rev        *v1.Revision
+		tlsEnabled bool
+		wantScheme corev1.URIScheme
+	}{{
+		name: "system internal TLS disabled",
+		rev: revision("bar", "foo",
+			withContainers([]corev1.Container{{
+				Name:           servingContainerName,
+				Image:          "busybox",
+				ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
+			}}),
+			WithContainerStatuses([]v1.ContainerStatus{{
+				ImageDigest: "busybox@sha256:deadbeef",
+			}}),
+		),
+		tlsEnabled: false,
+		wantScheme: corev1.URISchemeHTTP,
+	}, {
+		name: "system internal TLS enabled",
+		rev: revision("bar", "foo",
+			withContainers([]corev1.Container{{
+				Name:           servingContainerName,
+				Image:          "busybox",
+				ReadinessProbe: withTCPReadinessProbe(v1.DefaultUserPort),
+			}}),
+			WithContainerStatuses([]v1.ContainerStatus{{
+				ImageDigest: "busybox@sha256:deadbeef",
+			}}),
+		),
+		tlsEnabled: true,
+		wantScheme: corev1.URISchemeHTTPS,
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			cfg := revConfig()
+			if test.tlsEnabled {
+				cfg.Network.SystemInternalTLS = netcfg.EncryptionEnabled
+			}
+			got, err := makePodSpec(test.rev, cfg)
+			if err != nil {
+				t.Fatal("makePodSpec returned error:", err)
+			}
+			// Check that all user containers have the correct lifecycle scheme
+			for _, container := range got.Containers {
+				if container.Name == QueueContainerName {
+					continue
+				}
+				if container.Lifecycle == nil || container.Lifecycle.PreStop == nil ||
+					container.Lifecycle.PreStop.HTTPGet == nil {
+					t.Errorf("Container %s missing PreStop HTTPGet", container.Name)
+					continue
+				}
+				if container.Lifecycle.PreStop.HTTPGet.Scheme != test.wantScheme {
+					t.Errorf("Container %s PreStop scheme = %v, want %v",
+						container.Name, container.Lifecycle.PreStop.HTTPGet.Scheme, test.wantScheme)
+				}
+			}
+		})
+	}
+}
+
+func TestMakeUserLifecycle(t *testing.T) {
+	tests := []struct {
+		name                     string
+		systemInternalTLSEnabled bool
+		wantScheme               corev1.URIScheme
+	}{{
+		name:                     "system internal TLS disabled",
+		systemInternalTLSEnabled: false,
+		wantScheme:               corev1.URISchemeHTTP,
+	}, {
+		name:                     "system internal TLS enabled",
+		systemInternalTLSEnabled: true,
+		wantScheme:               corev1.URISchemeHTTPS,
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := makeUserLifecycle(test.systemInternalTLSEnabled)
+			if got == nil {
+				t.Fatal("makeUserLifecycle returned nil")
+			}
+			if got.PreStop == nil {
+				t.Fatal("makeUserLifecycle returned nil PreStop")
+			}
+			if got.PreStop.HTTPGet == nil {
+				t.Fatal("makeUserLifecycle returned nil HTTPGet")
+			}
+			if got.PreStop.HTTPGet.Scheme != test.wantScheme {
+				t.Errorf("makeUserLifecycle scheme = %v, want %v", got.PreStop.HTTPGet.Scheme, test.wantScheme)
+			}
+			// Verify other PreStop hook properties remain the same
+			if got.PreStop.HTTPGet.Port.IntValue() != networking.QueueAdminPort {
+				t.Errorf("makeUserLifecycle port = %v, want %v", got.PreStop.HTTPGet.Port.IntValue(), networking.QueueAdminPort)
+			}
+			if got.PreStop.HTTPGet.Path != queue.RequestQueueDrainPath {
+				t.Errorf("makeUserLifecycle path = %v, want %v", got.PreStop.HTTPGet.Path, queue.RequestQueueDrainPath)
+			}
+		})
+	}
+}
+
 var quantityComparer = cmp.Comparer(func(x, y resource.Quantity) bool {
 	return x.Cmp(y) == 0
 })