Don't allow port to conflict with QueueProxy (#2753)

* Don't allow port to conflict with QueueProxy This change prevents the user from choosing ports 8012 nad 8022 as they are currently reserved by the QueueProxy sidecar container. Ref #2752 Ref #2642 * Move and use constants in webhook
knative · Dec 21, 2018 · 5f2cc6f · 5f2cc6f
1 parent 6bb62bc
commit 5f2cc6f
Show file tree

Hide file tree

Showing 9 changed files with 54 additions and 31 deletions.
diff --git a/cmd/queue/main.go b/cmd/queue/main.go
@@ -35,6 +35,7 @@ import (
 	"github.com/knative/pkg/websocket"
 	"github.com/knative/serving/cmd/util"
 	activatorutil "github.com/knative/serving/pkg/activator/util"
+	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/autoscaler"
 	"github.com/knative/serving/pkg/http/h2c"
 	"github.com/knative/serving/pkg/logging"
@@ -314,13 +315,13 @@ func main() {
 	}, time.Now())
 
 	adminServer := &http.Server{
-		Addr:    fmt.Sprintf(":%d", queue.RequestQueueAdminPort),
+		Addr:    fmt.Sprintf(":%d", v1alpha1.RequestQueueAdminPort),
 		Handler: nil,
 	}
 	setupAdminHandlers(adminServer)
 
 	server = h2c.NewServer(
-		fmt.Sprintf(":%d", queue.RequestQueuePort),
+		fmt.Sprintf(":%d", v1alpha1.RequestQueuePort),
 		queue.TimeToFirstByteTimeoutHandler(http.HandlerFunc(handler), time.Duration(revisionTimeoutSeconds)*time.Second, "request timeout"))
 
 	// An `ErrServerClosed` should not trigger an early exit of

diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go
@@ -123,9 +123,26 @@ const (
 	// Deployment and Pod created by a Revision. This name will be set regardless of if
 	// a user specifies a port or the default value is chosen.
 	UserPortName = "user-port"
+
 	// DefaultUserPort is the default port value the QueueProxy will
 	// use for connecting to the user container.
 	DefaultUserPort = 8080
+
+	// RequestQueuePortName specifies the port name to use for http requests
+	// in queue-proxy container.
+	RequestQueuePortName string = "queue-port"
+
+	// RequestQueuePort specifies the port number to use for http requests
+	// in queue-proxy container.
+	RequestQueuePort = 8012
+
+	// RequestQueueAdminPortName specifies the port name for
+	// health check and lifecyle hooks for queue-proxy.
+	RequestQueueAdminPortName string = "queueadm-port"
+
+	// RequestQueueAdminPort specifies the port number for
+	// health check and lifecyle hooks for queue-proxy.
+	RequestQueueAdminPort = 8022
 )
 
 // RevisionSpec holds the desired state of the Revision (from the client).

diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go
@@ -198,6 +198,11 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError {
 		errs = errs.Also(apis.ErrDisallowedFields(disallowedFields...))
 	}
 
+	// Don't allow userPort to conflict with QueueProxy sidecar
+	if userPort.ContainerPort == RequestQueuePort || userPort.ContainerPort == RequestQueueAdminPort {
+		errs = errs.Also(apis.ErrInvalidValue(strconv.Itoa(int(userPort.ContainerPort)), "ContainerPort"))
+	}
+
 	if userPort.ContainerPort < 1 || userPort.ContainerPort > 65535 {
 		errs = errs.Also(apis.ErrOutOfBoundsValue(strconv.Itoa(int(userPort.ContainerPort)), "1", "65535", "ContainerPort"))
 	}

diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go
@@ -190,6 +190,24 @@ func TestContainerValidation(t *testing.T) {
 			}},
 		},
 		want: apis.ErrDisallowedFields("ports.HostIP"),
+	}, {
+		name: "port conflicts with queue proxy admin",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				ContainerPort: 8022,
+			}},
+		},
+		want: apis.ErrInvalidValue("8022", "ports.ContainerPort"),
+	}, {
+		name: "port conflicts with queue proxy",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				ContainerPort: 8012,
+			}},
+		},
+		want: apis.ErrInvalidValue("8012", "ports.ContainerPort"),
 	}, {
 		name: "has invalid port name",
 		c: corev1.Container{

diff --git a/pkg/queue/constants.go b/pkg/queue/constants.go
@@ -17,22 +17,6 @@ limitations under the License.
 package queue
 
 const (
-	// RequestQueuePortName specifies the port name to use for http requests
-	// in queue-proxy container.
-	RequestQueuePortName string = "queue-port"
-
-	// RequestQueuePort specifies the port number to use for http requests
-	// in queue-proxy container.
-	RequestQueuePort = 8012
-
-	// RequestQueueAdminPortName specifies the port name for
-	// health check and lifecyle hooks for queue-proxy.
-	RequestQueueAdminPortName string = "queueadm-port"
-
-	// RequestQueueAdminPort specifies the port number for
-	// health check and lifecyle hooks for queue-proxy.
-	RequestQueueAdminPort = 8022
-
 	// RequestQueueQuitPath specifies the path to send quit request to
 	// queue-proxy. This is used for preStop hook of queue-proxy. It:
 	// - marks the service as not ready, so that requests will no longer

diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go
@@ -61,7 +61,7 @@ var (
 	userLifecycle = &corev1.Lifecycle{
 		PreStop: &corev1.Handler{
 			HTTPGet: &corev1.HTTPGetAction{
-				Port: intstr.FromInt(queue.RequestQueueAdminPort),
+				Port: intstr.FromInt(v1alpha1.RequestQueueAdminPort),
 				Path: queue.RequestQueueQuitPath,
 			},
 		},
@@ -76,7 +76,7 @@ func rewriteUserProbe(p *corev1.Probe, userPort int) {
 	case p.HTTPGet != nil:
 		// For HTTP probes, we route them through the queue container
 		// so that we know the queue proxy is ready/live as well.
-		p.HTTPGet.Port = intstr.FromInt(queue.RequestQueuePort)
+		p.HTTPGet.Port = intstr.FromInt(v1alpha1.RequestQueuePort)
 	case p.TCPSocket != nil:
 		p.TCPSocket.Port = intstr.FromInt(userPort)
 	}

diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go
@@ -26,7 +26,6 @@ import (
 	"github.com/knative/serving/pkg/apis/serving"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/knative/serving/pkg/autoscaler"
-	"github.com/knative/serving/pkg/queue"
 	"github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
@@ -463,7 +462,7 @@ func TestMakePodSpec(t *testing.T) {
 				ReadinessProbe: &corev1.Probe{
 					Handler: corev1.Handler{
 						HTTPGet: &corev1.HTTPGetAction{
-							Port: intstr.FromInt(queue.RequestQueuePort),
+							Port: intstr.FromInt(v1alpha1.RequestQueuePort),
 							Path: "/",
 						},
 					},
@@ -670,7 +669,7 @@ func TestMakePodSpec(t *testing.T) {
 						HTTPGet: &corev1.HTTPGetAction{
 							Path: "/",
 							// HTTP probes route through the queue
-							Port: intstr.FromInt(queue.RequestQueuePort),
+							Port: intstr.FromInt(v1alpha1.RequestQueuePort),
 						},
 					},
 				},

diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue.go b/pkg/reconciler/v1alpha1/revision/resources/queue.go
@@ -36,27 +36,27 @@ var (
 		},
 	}
 	queuePorts = []corev1.ContainerPort{{
-		Name:          queue.RequestQueuePortName,
-		ContainerPort: int32(queue.RequestQueuePort),
+		Name:          v1alpha1.RequestQueuePortName,
+		ContainerPort: int32(v1alpha1.RequestQueuePort),
 	}, {
 		// Provides health checks and lifecycle hooks.
-		Name:          queue.RequestQueueAdminPortName,
-		ContainerPort: int32(queue.RequestQueueAdminPort),
+		Name:          v1alpha1.RequestQueueAdminPortName,
+		ContainerPort: int32(v1alpha1.RequestQueueAdminPort),
 	}}
 	// This handler (1) marks the service as not ready and (2)
 	// adds a small delay before the container is killed.
 	queueLifecycle = &corev1.Lifecycle{
 		PreStop: &corev1.Handler{
 			HTTPGet: &corev1.HTTPGetAction{
-				Port: intstr.FromInt(queue.RequestQueueAdminPort),
+				Port: intstr.FromInt(v1alpha1.RequestQueueAdminPort),
 				Path: queue.RequestQueueQuitPath,
 			},
 		},
 	}
 	queueReadinessProbe = &corev1.Probe{
 		Handler: corev1.Handler{
 			HTTPGet: &corev1.HTTPGetAction{
-				Port: intstr.FromInt(queue.RequestQueueAdminPort),
+				Port: intstr.FromInt(v1alpha1.RequestQueueAdminPort),
 				Path: queue.RequestQueueHealthPath,
 			},
 		},

diff --git a/pkg/reconciler/v1alpha1/revision/resources/service.go b/pkg/reconciler/v1alpha1/revision/resources/service.go
@@ -21,7 +21,6 @@ import (
 	"github.com/knative/serving/pkg/apis/autoscaling"
 	"github.com/knative/serving/pkg/apis/serving"
 	"github.com/knative/serving/pkg/apis/serving/v1alpha1"
-	"github.com/knative/serving/pkg/queue"
 	"github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources/names"
 
 	corev1 "k8s.io/api/core/v1"
@@ -34,7 +33,7 @@ var (
 		Name:       ServicePortName,
 		Protocol:   corev1.ProtocolTCP,
 		Port:       ServicePort,
-		TargetPort: intstr.IntOrString{Type: intstr.String, StrVal: queue.RequestQueuePortName},
+		TargetPort: intstr.IntOrString{Type: intstr.String, StrVal: v1alpha1.RequestQueuePortName},
 	}}
 )