Allow user-defined ports on Configuration (#2642)

This change allows for a user to add a port to their Container object
in Configuration. The single port provided is used by the queue-proxy to
as the port used to connect to the user container. This port is
available as environment variable "PORT" on the user container and
environment variable "USER_PORT" on the queue-proxy.

This change is built upon PR #2190 by Leon-Liangming. It squashes the
following commits:

* change queue-proxy user-port env name
* rebase master, fix uint test
* remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false
* add e2e test for user-port && review comments modify && open acitvator's istio-proxy
* set more validation info about user-port && modify e2e test
* change validation info && e2e test
* more review comments modify
* Allow only 1 unnamed port
* Passing conformance tests

Related to #2258

Reduce number of type conversions

Allow h2c and http1 on for port name

Code Review Comments

Clean up port validation

cmd/queue/main.go

@@ -66,7 +66,8 @@ var (
 	servingRevision        string
 	servingRevisionKey     string
 	servingAutoscaler      string
-	servingAutoscalerPort  string
+	servingAutoscalerPort  int
+	userTargetPort         int
 	containerConcurrency   int
 	revisionTimeoutSeconds int
 	statChan               = make(chan *autoscaler.Stat, statReportingQueueLength)
@@ -89,9 +90,10 @@ func initEnv() {
 	servingNamespace = util.GetRequiredEnvOrFatal("SERVING_NAMESPACE", logger)
 	servingRevision = util.GetRequiredEnvOrFatal("SERVING_REVISION", logger)
 	servingAutoscaler = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER", logger)
-	servingAutoscalerPort = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER_PORT", logger)
+	servingAutoscalerPort = util.MustParseIntEnvOrFatal("SERVING_AUTOSCALER_PORT", logger)
 	containerConcurrency = util.MustParseIntEnvOrFatal("CONTAINER_CONCURRENCY", logger)
 	revisionTimeoutSeconds = util.MustParseIntEnvOrFatal("REVISION_TIMEOUT_SECONDS", logger)
+	userTargetPort = util.MustParseIntEnvOrFatal("USER_PORT", logger)
 
 	// TODO(mattmoor): Move this key to be in terms of the KPA.
 	servingRevisionKey = autoscaler.NewKpaKey(servingNamespace, servingRevision)
@@ -261,7 +263,7 @@ func main() {
 		zap.String(logkey.Key, servingRevisionKey),
 		zap.String(logkey.Pod, podName))
 
-	target, err := url.Parse("http://localhost:8080")
+	target, err := url.Parse(fmt.Sprintf("http://localhost:%d", userTargetPort))
 	if err != nil {
 		logger.Fatal("Failed to parse localhost url", zap.Error(err))
 	}
@@ -299,7 +301,7 @@ func main() {
 	}()
 
 	// Open a websocket connection to the autoscaler
-	autoscalerEndpoint := fmt.Sprintf("ws://%s.%s:%s", servingAutoscaler, system.Namespace, servingAutoscalerPort)
+	autoscalerEndpoint := fmt.Sprintf("ws://%s.%s:%d", servingAutoscaler, system.Namespace, servingAutoscalerPort)
 	logger.Infof("Connecting to autoscaler at %s", autoscalerEndpoint)
 	statSink = websocket.NewDurableSendingConnection(autoscalerEndpoint)
 	go statReporter()

pkg/apis/serving/v1alpha1/revision_types.go

@@ -118,6 +118,16 @@ const (
 	RevisionContainerConcurrencyMax RevisionContainerConcurrencyType = 1000
 )
 
+const (
+	// UserPortName is the name that will be used for the Port on the
+	// 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
+)
+
 // RevisionSpec holds the desired state of the Revision (from the client).
 type RevisionSpec struct {
 	// TODO: Generation does not work correctly with CRD. They are scrubbed

pkg/apis/serving/v1alpha1/revision_validation.go

@@ -130,9 +130,6 @@ func validateContainer(container corev1.Container) *apis.FieldError {
 	if container.Name != "" {
 		ignoredFields = append(ignoredFields, "name")
 	}
-	if len(container.Ports) > 0 {
-		ignoredFields = append(ignoredFields, "ports")
-	}
 	if len(container.VolumeMounts) > 0 {
 		ignoredFields = append(ignoredFields, "volumeMounts")
 	}
@@ -144,6 +141,9 @@ func validateContainer(container corev1.Container) *apis.FieldError {
 		// Complain about all ignored fields so that user can remove them all at once.
 		errs = errs.Also(apis.ErrDisallowedFields(ignoredFields...))
 	}
+	if err := validateContainerPorts(container.Ports); err != nil {
+		errs = errs.Also(err.ViaField("ports"))
+	}
 	// Validate our probes
 	if err := validateProbe(container.ReadinessProbe).ViaField("readinessProbe"); err != nil {
 		errs = errs.Also(err)
@@ -162,6 +162,68 @@ func validateContainer(container corev1.Container) *apis.FieldError {
 	return errs
 }
 
+func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError {
+	if len(ports) == 0 {
+		return nil
+	}
+
+	var errs *apis.FieldError
+
+	// user can set container port which names "user-port" to define application's port.
+	// Queue-proxy will use it to send requests to application
+	// if user didn't set any port, it will set default port user-port=8080.
+	if len(ports) > 1 {
+		errs = errs.Also(&apis.FieldError{
+			Message: "More than one container port is set",
+			Paths:   []string{apis.CurrentField},
+			Details: "Only a single port is allowed",
+		})
+	}
+
+	userPort := ports[0]
+	// Only allow empty (defaulting to "TCP") or explicit TCP for protocol
+	if userPort.Protocol != "" && userPort.Protocol != corev1.ProtocolTCP {
+		errs = errs.Also(apis.ErrInvalidValue(string(userPort.Protocol), "Protocol"))
+	}
+
+	// Don't allow HostIP or HostPort to be set
+	var disallowedFields []string
+	if userPort.HostIP != "" {
+		disallowedFields = append(disallowedFields, "HostIP")
+
+	}
+	if userPort.HostPort != 0 {
+		disallowedFields = append(disallowedFields, "HostPort")
+	}
+	if len(disallowedFields) != 0 {
+		errs = errs.Also(apis.ErrDisallowedFields(disallowedFields...))
+	}
+
+	if userPort.ContainerPort < 1 || userPort.ContainerPort > 65535 {
+		errs = errs.Also(apis.ErrOutOfBoundsValue(strconv.Itoa(int(userPort.ContainerPort)), "1", "65535", "ContainerPort"))
+	}
+
+	// The port is named "user-port" on the deployment, but a user cannot set an arbitrary name on the port
+	// in Configuration. The name field is reserved for content-negotiation. Currently 'h2c' and 'http1' are
+	// allowed.
+	// https://github.com/knative/serving/blob/master/docs/runtime-contract.md#inbound-network-connectivity
+	validPortNames := map[string]bool{
+		"h2c":   true,
+		"http1": true,
+		"":      true,
+	}
+
+	if !validPortNames[userPort.Name] {
+		errs = errs.Also(&apis.FieldError{
+			Message: fmt.Sprintf("Port name %v is not allowed", ports[0].Name),
+			Paths:   []string{apis.CurrentField},
+			Details: "Name must be empty, or one of: 'h2c', 'http1'",
+		})
+	}
+
+	return errs
+}
+
 func validateBuildRef(buildRef *corev1.ObjectReference) *apis.FieldError {
 	if buildRef == nil {
 		return nil

pkg/apis/serving/v1alpha1/revision_validation_test.go

@@ -77,15 +77,134 @@ func TestContainerValidation(t *testing.T) {
 		},
 		want: nil,
 	}, {
-		name: "has ports",
+		name: "has no container ports set",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{},
+		},
+		want: nil,
+	}, {
+		name: "has valid user port http1",
 		c: corev1.Container{
 			Image: "foo",
 			Ports: []corev1.ContainerPort{{
-				Name:          "http",
+				Name:          "http1",
+				ContainerPort: 8081,
+			}},
+		},
+		want: nil,
+	}, {
+		name: "has valid user port h2c",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				Name:          "h2c",
+				ContainerPort: 8081,
+			}},
+		},
+		want: nil,
+	}, {
+		name: "has more than one ports with valid names",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				Name:          "h2c",
 				ContainerPort: 8080,
+			}, {
+				Name:          "http1",
+				ContainerPort: 8181,
 			}},
 		},
-		want: apis.ErrDisallowedFields("ports"),
+		want: &apis.FieldError{
+			Message: "More than one container port is set",
+			Paths:   []string{"ports"},
+			Details: "Only a single port is allowed",
+		},
+	}, {
+		name: "has container port value too large",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				ContainerPort: 65536,
+			}},
+		},
+		want: apis.ErrOutOfBoundsValue("65536", "1", "65535", "ports.ContainerPort"),
+	}, {
+		name: "has an empty port set",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{}},
+		},
+		want: apis.ErrOutOfBoundsValue("0", "1", "65535", "ports.ContainerPort"),
+	}, {
+		name: "has more than one unnamed port",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				ContainerPort: 8080,
+			}, {
+				ContainerPort: 8181,
+			}},
+		},
+		want: &apis.FieldError{
+			Message: "More than one container port is set",
+			Paths:   []string{"ports"},
+			Details: "Only a single port is allowed",
+		},
+	}, {
+		name: "has tcp protocol",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				Protocol:      corev1.ProtocolTCP,
+				ContainerPort: 8080,
+			}},
+		},
+		want: nil,
+	}, {
+		name: "has invalid protocol",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				Protocol:      "tdp",
+				ContainerPort: 8080,
+			}},
+		},
+		want: apis.ErrInvalidValue("tdp", "ports.Protocol"),
+	}, {
+		name: "has host port",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				HostPort:      80,
+				ContainerPort: 8080,
+			}},
+		},
+		want: apis.ErrDisallowedFields("ports.HostPort"),
+	}, {
+		name: "has host ip",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				HostIP:        "127.0.0.1",
+				ContainerPort: 8080,
+			}},
+		},
+		want: apis.ErrDisallowedFields("ports.HostIP"),
+	}, {
+		name: "has invalid port name",
+		c: corev1.Container{
+			Image: "foo",
+			Ports: []corev1.ContainerPort{{
+				Name:          "foobar",
+				ContainerPort: 8080,
+			}},
+		},
+		want: &apis.FieldError{
+			Message: fmt.Sprintf("Port name %v is not allowed", "foobar"),
+			Paths:   []string{"ports"},
+			Details: "Name must be empty, or one of: 'h2c', 'http1'",
+		},
 	}, {
 		name: "has volumeMounts",
 		c: corev1.Container{
@@ -152,17 +271,13 @@ func TestContainerValidation(t *testing.T) {
 		name: "has numerous problems",
 		c: corev1.Container{
 			Name: "foo",
-			Ports: []corev1.ContainerPort{{
-				Name:          "http",
-				ContainerPort: 8080,
-			}},
 			VolumeMounts: []corev1.VolumeMount{{
 				MountPath: "mount/path",
 				Name:      "name",
 			}},
 			Lifecycle: &corev1.Lifecycle{},
 		},
-		want: apis.ErrDisallowedFields("name", "ports", "volumeMounts", "lifecycle").Also(
+		want: apis.ErrDisallowedFields("name", "volumeMounts", "lifecycle").Also(
 			&apis.FieldError{
 				Message: "Failed to parse image reference",
 				Paths:   []string{"image"},

pkg/reconciler/v1alpha1/revision/resources/constants.go

@@ -30,8 +30,6 @@ const (
 	// TODO(mattmoor): Make this private once we remove revision_test.go
 	IstioOutboundIPRangeAnnotation = "traffic.sidecar.istio.io/includeOutboundIPRanges"
 
-	userPortName    = "user-port"
-	userPort        = 8080
 	userPortEnvName = "PORT"
 
 	autoscalerPort = 8080

pkg/reconciler/v1alpha1/revision/resources/deploy.go

@@ -48,17 +48,6 @@ var (
 		MountPath: "/var/log",
 	}
 
-	userPorts = []corev1.ContainerPort{{
-		Name:          userPortName,
-		ContainerPort: int32(userPort),
-	}}
-
-	// Expose containerPort as env PORT.
-	userEnv = corev1.EnvVar{
-		Name:  userPortEnvName,
-		Value: strconv.Itoa(userPort),
-	}
-
 	userResources = corev1.ResourceRequirements{
 		Requests: corev1.ResourceList{
 			corev1.ResourceCPU: userContainerCPU,
@@ -79,7 +68,7 @@ var (
 	}
 )
 
-func rewriteUserProbe(p *corev1.Probe) {
+func rewriteUserProbe(p *corev1.Probe, userPort int) {
 	if p == nil {
 		return
 	}
@@ -123,19 +112,24 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab
 	// If client provides for some resources, override default values
 	applyDefaultResources(userResources, &userContainer.Resources)
 
-	userContainer.Ports = userPorts
 	userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount)
 	userContainer.Lifecycle = userLifecycle
-	userContainer.Env = append(userContainer.Env, userEnv)
+	userPort := getUserPort(rev)
+	userPortInt := int(userPort)
+	userPortStr := strconv.Itoa(userPortInt)
+	// Replacement is safe as only up to a single port is allowed on the Revision
+	userContainer.Ports = buildContainerPorts(userPort)
+	userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPortStr))
 	userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...)
+
 	// Prefer imageDigest from revision if available
 	if rev.Status.ImageDigest != "" {
 		userContainer.Image = rev.Status.ImageDigest
 	}
 
 	// If the client provides probes, we should fill in the port for them.
-	rewriteUserProbe(userContainer.ReadinessProbe)
-	rewriteUserProbe(userContainer.LivenessProbe)
+	rewriteUserProbe(userContainer.ReadinessProbe, userPortInt)
+	rewriteUserProbe(userContainer.LivenessProbe, userPortInt)
 
 	revisionTimeout := int64(rev.Spec.TimeoutSeconds.Duration.Seconds())
 
@@ -158,6 +152,30 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab
 	return podSpec
 }
 
+func getUserPort(rev *v1alpha1.Revision) int32 {
+	if len(rev.Spec.Container.Ports) == 1 {
+		return rev.Spec.Container.Ports[0].ContainerPort
+	}
+
+	//TODO(#2258): Use container EXPOSE metadata from image before falling back to default value
+
+	return v1alpha1.DefaultUserPort
+}
+
+func buildContainerPorts(userPort int32) []corev1.ContainerPort {
+	return []corev1.ContainerPort{{
+		Name:          v1alpha1.UserPortName,
+		ContainerPort: userPort,
+	}}
+}
+
+func buildUserPortEnv(userPort string) corev1.EnvVar {
+	return corev1.EnvVar{
+		Name:  userPortEnvName,
+		Value: userPort,
+	}
+}
+
 func MakeDeployment(rev *v1alpha1.Revision,
 	loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability,
 	autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *appsv1.Deployment {