From 62193cd747e27b9440df153af3700d272692e74a Mon Sep 17 00:00:00 2001 From: l00388569 Date: Sat, 29 Sep 2018 17:22:59 +0800 Subject: [PATCH] Allow user-defined ports on Configuration 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 | 10 +- pkg/apis/serving/v1alpha1/revision_types.go | 10 ++ .../serving/v1alpha1/revision_validation.go | 68 ++++++- .../v1alpha1/revision_validation_test.go | 131 +++++++++++++- .../v1alpha1/revision/resources/constants.go | 2 - .../v1alpha1/revision/resources/deploy.go | 50 ++++-- .../revision/resources/deploy_test.go | 169 +++++++++++++++--- .../v1alpha1/revision/resources/queue.go | 4 + .../v1alpha1/revision/resources/queue_test.go | 49 ++++- test/configuration.go | 1 + test/conformance/service_test.go | 48 ++++- test/conformance/util.go | 11 +- test/crd.go | 3 + test/e2e/helloworld_shell_test.go | 4 +- test/service.go | 11 ++ test/test_images/printport/README.md | 21 +++ test/test_images/printport/main.go | 43 +++++ test/test_images/printport/printport.yaml | 47 +++++ 18 files changed, 611 insertions(+), 71 deletions(-) create mode 100644 test/test_images/printport/README.md create mode 100644 test/test_images/printport/main.go create mode 100644 test/test_images/printport/printport.yaml diff --git a/cmd/queue/main.go b/cmd/queue/main.go index c90a3cf16927..94c158013906 100644 --- a/cmd/queue/main.go +++ b/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() diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index d680b5dabbdc..b02b775c4adb 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/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 diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go index 4a5d89680cff..b9dd84b0ee33 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation.go +++ b/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 diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index 4200f81917cf..a7ff52bd7b70 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/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"}, diff --git a/pkg/reconciler/v1alpha1/revision/resources/constants.go b/pkg/reconciler/v1alpha1/revision/resources/constants.go index e9617b804f0a..e7462b973863 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/constants.go +++ b/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 diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go index 43d4d3e696fc..24ea5c30b074 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy.go +++ b/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 { diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go index b31011882331..5f075b032d99 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "strconv" "testing" "time" @@ -36,7 +37,8 @@ import ( ) var ( - one int32 = 1 + one int32 = 1 + defaultPortStr string = strconv.Itoa(int(v1alpha1.DefaultUserPort)) ) func refInt64(num int64) *int64 { @@ -54,6 +56,106 @@ func TestMakePodSpec(t *testing.T) { cc *config.Controller want *corev1.PodSpec }{{ + name: "user-defined user port, queue proxy have PORT env", + rev: &v1alpha1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "foo", + Name: "bar", + UID: "1234", + Labels: labels, + }, + Spec: v1alpha1.RevisionSpec{ + ContainerConcurrency: 1, + TimeoutSeconds: &metav1.Duration{ + Duration: 45 * time.Second, + }, + Container: corev1.Container{ + Image: "busybox", + Ports: []corev1.ContainerPort{ + { + ContainerPort: 8888, + }, + }, + }, + }, + }, + lc: &logging.Config{}, + oc: &config.Observability{}, + ac: &autoscaler.Config{}, + cc: &config.Controller{}, + want: &corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: userContainerName, + Image: "busybox", + Resources: userResources, + Ports: []corev1.ContainerPort{ + { + Name: v1alpha1.UserPortName, + ContainerPort: 8888, + }, + }, + VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, + Lifecycle: userLifecycle, + Env: []corev1.EnvVar{{ + Name: "PORT", + Value: "8888", // match user port + }, { + Name: "K_REVISION", + Value: "bar", + }, { + Name: "K_CONFIGURATION", + Value: "cfg", + }, { + Name: "K_SERVICE", + Value: "svc", + }}, + }, { + Name: queueContainerName, + Resources: queueResources, + Ports: queuePorts, + Lifecycle: queueLifecycle, + ReadinessProbe: queueReadinessProbe, + Env: []corev1.EnvVar{{ + Name: "SERVING_NAMESPACE", + Value: "foo", // matches namespace + }, { + Name: "SERVING_CONFIGURATION", + // No OwnerReference + }, { + Name: "SERVING_REVISION", + Value: "bar", // matches name + }, { + Name: "SERVING_AUTOSCALER", + Value: "autoscaler", // no autoscaler configured. + }, { + Name: "SERVING_AUTOSCALER_PORT", + Value: "8080", + }, { + Name: "CONTAINER_CONCURRENCY", + Value: "1", + }, { + Name: "REVISION_TIMEOUT_SECONDS", + Value: "45", + }, { + Name: "SERVING_POD", + ValueFrom: &corev1.EnvVarSource{ + FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}, + }, + }, { + Name: "SERVING_LOGGING_CONFIG", + // No logging configuration + }, { + Name: "SERVING_LOGGING_LEVEL", + // No logging level + }, { + Name: "USER_PORT", + Value: "8888", // Match user port + }}, + }}, + Volumes: []corev1.Volume{varLogVolume}, + TerminationGracePeriodSeconds: refInt64(45), + }, + }, { name: "simple concurrency=single no owner", rev: &v1alpha1.Revision{ ObjectMeta: metav1.ObjectMeta{ @@ -81,10 +183,10 @@ func TestMakePodSpec(t *testing.T) { Name: userContainerName, Image: "busybox", Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -134,6 +236,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -170,10 +275,10 @@ func TestMakePodSpec(t *testing.T) { Name: userContainerName, Image: "busybox@sha256:deadbeef", Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -223,6 +328,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -263,10 +371,10 @@ func TestMakePodSpec(t *testing.T) { Name: userContainerName, Image: "busybox", Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -316,6 +424,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -337,7 +448,7 @@ func TestMakePodSpec(t *testing.T) { ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ - Port: intstr.FromInt(userPort), + Port: intstr.FromInt(v1alpha1.DefaultUserPort), Path: "/", }, }, @@ -365,10 +476,10 @@ func TestMakePodSpec(t *testing.T) { }, }, Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -418,6 +529,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -465,10 +579,10 @@ func TestMakePodSpec(t *testing.T) { }, }, Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -518,6 +632,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -567,10 +684,10 @@ func TestMakePodSpec(t *testing.T) { }, }, Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -620,6 +737,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -660,15 +780,15 @@ func TestMakePodSpec(t *testing.T) { LivenessProbe: &corev1.Probe{ Handler: corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(userPort), + Port: intstr.FromInt(v1alpha1.DefaultUserPort), }, }, }, Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -718,6 +838,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -754,10 +877,10 @@ func TestMakePodSpec(t *testing.T) { Name: userContainerName, Image: "busybox", Resources: userResources, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, - Env: []corev1.EnvVar{userEnv, + Env: []corev1.EnvVar{buildUserPortEnv(defaultPortStr), { Name: "K_REVISION", Value: "bar", @@ -807,6 +930,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }, { Name: fluentdContainerName, @@ -925,7 +1051,7 @@ func TestMakePodSpec(t *testing.T) { corev1.ResourceCPU: resource.MustParse("888m"), }, }, - Ports: userPorts, + Ports: buildContainerPorts(v1alpha1.DefaultUserPort), VolumeMounts: []corev1.VolumeMount{varLogVolumeMount}, Lifecycle: userLifecycle, }, { @@ -967,6 +1093,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue.go b/pkg/reconciler/v1alpha1/revision/resources/queue.go index 3e8a319aba6f..e7809fa21eeb 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue.go @@ -77,6 +77,7 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a } autoscalerAddress := "autoscaler" + userPort := getUserPort(rev) var loggingLevel string if ll, ok := loggingConfig.LoggingLevel["queueproxy"]; ok { @@ -124,6 +125,9 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a }, { Name: "SERVING_LOGGING_LEVEL", Value: loggingLevel, + }, { + Name: "USER_PORT", + Value: strconv.Itoa(int(userPort)), }}, } } diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go index 0a6534784ac1..8e3e594c5e3b 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go @@ -17,6 +17,7 @@ limitations under the License. package resources import ( + "strconv" "testing" "time" @@ -36,12 +37,13 @@ var boolTrue = true func TestMakeQueueContainer(t *testing.T) { tests := []struct { - name string - rev *v1alpha1.Revision - lc *logging.Config - ac *autoscaler.Config - cc *config.Controller - want *corev1.Container + name string + rev *v1alpha1.Revision + lc *logging.Config + ac *autoscaler.Config + cc *config.Controller + userport *corev1.ContainerPort + want *corev1.Container }{{ name: "no owner no autoscaler single", rev: &v1alpha1.Revision{ @@ -60,6 +62,10 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.ContainerPort{ + Name: userPortEnvName, + ContainerPort: v1alpha1.DefaultUserPort, + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -100,6 +106,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: strconv.Itoa(v1alpha1.DefaultUserPort), }}, }, }, { @@ -122,6 +131,10 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{ QueueSidecarImage: "alpine", }, + userport: &corev1.ContainerPort{ + Name: userPortEnvName, + ContainerPort: v1alpha1.DefaultUserPort, + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -163,6 +176,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: strconv.Itoa(v1alpha1.DefaultUserPort), }}, }, }, { @@ -190,6 +206,10 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.ContainerPort{ + Name: userPortEnvName, + ContainerPort: v1alpha1.DefaultUserPort, + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -230,6 +250,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: strconv.Itoa(v1alpha1.DefaultUserPort), }}, }, }, { @@ -255,6 +278,10 @@ func TestMakeQueueContainer(t *testing.T) { }, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.ContainerPort{ + Name: userPortEnvName, + ContainerPort: v1alpha1.DefaultUserPort, + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -295,6 +322,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", Value: "error", // from logging config + }, { + Name: "USER_PORT", + Value: strconv.Itoa(v1alpha1.DefaultUserPort), }}, }, }, { @@ -315,6 +345,10 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.ContainerPort{ + Name: userPortEnvName, + ContainerPort: v1alpha1.DefaultUserPort, + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -355,6 +389,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: strconv.Itoa(v1alpha1.DefaultUserPort), }}, }, }} diff --git a/test/configuration.go b/test/configuration.go index 1da90d34a71d..8cd330bc4174 100644 --- a/test/configuration.go +++ b/test/configuration.go @@ -30,6 +30,7 @@ import ( // Options are test setup parameters. type Options struct { EnvVars []corev1.EnvVar + ContainerPorts []corev1.ContainerPort ContainerConcurrency int RevisionTimeout time.Duration ContainerResources corev1.ResourceRequirements diff --git a/test/conformance/service_test.go b/test/conformance/service_test.go index 4799db066299..c787ab049310 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -21,15 +21,21 @@ package conformance import ( "fmt" "net/http" + "strconv" "testing" pkgTest "github.com/knative/pkg/test" "github.com/knative/pkg/test/logging" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/test" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +const ( + userPort = int32(8888) +) + // Validates the state of Configuration, Revision, and Route objects for a runLatest Service. The checks in this method should be able to be performed at any point in a // runLatest Service's lifecycle so long as the service is in a "Ready" state. func validateRunLatestControlPlane(logger *logging.BaseLogger, clients *test.Clients, names test.ResourceNames, expectedGeneration string) error { @@ -80,6 +86,7 @@ func validateRunLatestControlPlane(logger *logging.BaseLogger, clients *test.Cli // Validates service health and vended content match for a runLatest Service. The checks in this method should be able to be performed at any point in a // runLatest Service's lifecycle so long as the service is in a "Ready" state. func validateRunLatestDataPlane(logger *logging.BaseLogger, clients *test.Clients, names test.ResourceNames, expectedText string) error { + logger.Infof("Checking that the endpoint vends the expected text: %s", expectedText) _, err := pkgTest.WaitForEndpointState( clients.KubeClient, logger, @@ -138,6 +145,7 @@ func validateLabelsPropagation(logger *logging.BaseLogger, objects test.Resource // 2. Update Metadata // a. Update Labels // b. Update Annotations +// 3. Update UserPort func TestRunLatestService(t *testing.T) { clients := setup(t) @@ -178,7 +186,7 @@ func TestRunLatestService(t *testing.T) { // Update Container Image logger.Info("Updating the Service to use a different image.") - names.Image = pizzaPlanet2 + names.Image = printport image2 := test.ImagePath(names.Image) if _, err := test.PatchServiceImage(logger, clients, objects.Service, image2); err != nil { t.Fatalf("Patch update for Service %s with new image %s failed: %v", names.Service, image2, err) @@ -195,9 +203,10 @@ func TestRunLatestService(t *testing.T) { if err != nil { t.Error(err) } - err = validateRunLatestDataPlane(logger, clients, names, pizzaPlanetText2) + err = validateRunLatestDataPlane(logger, clients, names, strconv.Itoa(v1alpha1.DefaultUserPort)) if err != nil { t.Error(err) + } // Update Metadata (Labels) @@ -234,7 +243,7 @@ func TestRunLatestService(t *testing.T) { logger.Info("Waiting for the new revision to appear as LatestRevision.") names.Revision, err = test.WaitForServiceLatestRevision(clients, names) if err != nil { - t.Fatalf("new image does not become ready in Service: %v", err) + t.Fatalf("The new revision has not become ready in Service: %v", err) } // Validate Service @@ -242,7 +251,7 @@ func TestRunLatestService(t *testing.T) { if err != nil { t.Error(err) } - err = validateRunLatestDataPlane(logger, clients, names, pizzaPlanetText2) + err = validateRunLatestDataPlane(logger, clients, names, strconv.Itoa(v1alpha1.DefaultUserPort)) if err != nil { t.Error(err) } @@ -250,6 +259,34 @@ func TestRunLatestService(t *testing.T) { if err := test.GetRouteProberError(routeProberErrorChan, logger); err != nil { t.Fatalf("Route prober failed with error %s", err) } + + // Update container with user port + logger.Infof("Updating the port of the user container for service %s", names.Service) + desiredSvc := objects.Service.DeepCopy() + desiredSvc.Spec.RunLatest.Configuration.RevisionTemplate.Spec.Container.Ports = []corev1.ContainerPort{{ + ContainerPort: userPort, + }} + objects.Service, err = test.PatchService(logger, clients, objects.Service, desiredSvc) + if err != nil { + t.Fatalf("Service %s was not updated with a new port for the user container: %v", names.Service, err) + } + + logger.Info("Waiting for the new revision to appear as LatestRevision.") + names.Revision, err = test.WaitForServiceLatestRevision(clients, names) + if err != nil { + t.Fatalf("The new revision has not become ready in Service: %v", err) + } + + // Validate Service + err = validateRunLatestControlPlane(logger, clients, names, "5") + if err != nil { + t.Error(err) + } + err = validateRunLatestDataPlane(logger, clients, names, strconv.Itoa(int(userPort))) + if err != nil { + t.Error(err) + } + } // TestReleaseService creates a Service in runLatest mode and then updates it to release mode. Once in release mode the test @@ -351,7 +388,4 @@ func TestReleaseService(t *testing.T) { []string{expectedThirdRev, expectedSecondRev, expectedFirstRev}) } -// TODO(jonjohnsonjr): LatestService roads less traveled. -// TODO(jonjohnsonjr): PinnedService happy path. -// TODO(jonjohnsonjr): PinnedService roads less traveled. // TODO(jonjohnsonjr): Examples of deploying from source. diff --git a/test/conformance/util.go b/test/conformance/util.go index 2681bda211f3..16cc2542d7d2 100644 --- a/test/conformance/util.go +++ b/test/conformance/util.go @@ -41,14 +41,12 @@ import ( // Constants for test images located in test/test_images const ( pizzaPlanet1 = "pizzaplanetv1" - pizzaPlanetText1 = "What a spaceport!" pizzaPlanet2 = "pizzaplanetv2" - pizzaPlanetText2 = "Re-energize yourself with a slice of pepperoni!" helloworld = "helloworld" - helloWorldText = "Hello World! How about some tasty noodles?" httpproxy = "httpproxy" singleThreadedImage = "singlethreaded" timeout = "timeout" + printport = "printport" concurrentRequests = 50 // We expect to see 100% of requests succeed for traffic sent directly to revisions. @@ -59,6 +57,13 @@ const ( minSplitPercentage = 0.25 ) +// Constants for test image output +const ( + pizzaPlanetText1 = "What a spaceport!" + pizzaPlanetText2 = "Re-energize yourself with a slice of pepperoni!" + helloWorldText = "Hello World! How about some tasty noodles?" +) + func setup(t *testing.T) *test.Clients { clients, err := test.NewClients(pkgTest.Flags.Kubeconfig, pkgTest.Flags.Cluster, test.ServingNamespace) if err != nil { diff --git a/test/crd.go b/test/crd.go index e5d114de7dca..241798555625 100644 --- a/test/crd.go +++ b/test/crd.go @@ -128,6 +128,9 @@ func Configuration(namespace string, names ResourceNames, imagePath string, opti }, Spec: *ConfigurationSpec(imagePath, options), } + if options.ContainerPorts != nil && len(options.ContainerPorts) > 0 { + config.Spec.RevisionTemplate.Spec.Container.Ports = options.ContainerPorts + } return config } diff --git a/test/e2e/helloworld_shell_test.go b/test/e2e/helloworld_shell_test.go index 9aad87e276e9..c5ba1a8d97bc 100644 --- a/test/e2e/helloworld_shell_test.go +++ b/test/e2e/helloworld_shell_test.go @@ -71,13 +71,13 @@ func TestHelloWorldFromShell(t *testing.T) { // Populate manifets file with the real path to the container yamlBytes, err := ioutil.ReadFile(appYaml) + if err != nil { t.Fatalf("Failed to read file %s: %v", appYaml, err) } content := strings.Replace(string(yamlBytes), yamlImagePlaceholder, imagePath, -1) - content = strings.Replace(string(content), "namespace: "+namespacePlaceholder, - "namespace: "+test.ServingNamespace, -1) + content = strings.Replace(string(content), namespacePlaceholder, test.ServingNamespace, -1) if _, err = newYaml.WriteString(content); err != nil { t.Fatalf("Failed to write new manifest: %v", err) diff --git a/test/service.go b/test/service.go index ef188894e788..829bed8accbf 100644 --- a/test/service.go +++ b/test/service.go @@ -176,6 +176,7 @@ func PatchServiceImage(logger *logging.BaseLogger, clients *Clients, svc *v1alph } else { return nil, fmt.Errorf("UpdateImageService(%v): unable to determine service type", svc) } + LogResourceObject(logger, ResourceObjects{Service: newSvc}) patchBytes, err := createPatch(svc, newSvc) if err != nil { return nil, err @@ -183,6 +184,16 @@ func PatchServiceImage(logger *logging.BaseLogger, clients *Clients, svc *v1alph return clients.ServingClient.Services.Patch(svc.ObjectMeta.Name, types.JSONPatchType, patchBytes, "") } +// PatchService creates and applies a patch from the diff between curSvc and desiredSvc. Returns the latest service object. +func PatchService(logger *logging.BaseLogger, clients *Clients, curSvc *v1alpha1.Service, desiredSvc *v1alpha1.Service) (*v1alpha1.Service, error) { + LogResourceObject(logger, ResourceObjects{Service: desiredSvc}) + patchBytes, err := createPatch(curSvc, desiredSvc) + if err != nil { + return nil, err + } + return clients.ServingClient.Services.Patch(curSvc.ObjectMeta.Name, types.JSONPatchType, patchBytes, "") +} + // PatchServiceRevisionTemplateMetadata patches an existing service by adding metadata to the service's RevisionTemplateSpec. func PatchServiceRevisionTemplateMetadata(logger *logging.BaseLogger, clients *Clients, svc *v1alpha1.Service, metadata metav1.ObjectMeta) (*v1alpha1.Service, error) { newSvc := svc.DeepCopy() diff --git a/test/test_images/printport/README.md b/test/test_images/printport/README.md new file mode 100644 index 000000000000..42cfb3198b44 --- /dev/null +++ b/test/test_images/printport/README.md @@ -0,0 +1,21 @@ +# PrintPort Test Image + +This directory contains the test image used in the user-port e2e test. + +The image contains a simple Go webserver, `printport.go`, that will listen on the port defined by environment variable `PORT` and expose a service at `/`. + +When called, the server emits the port it was called on. + +See the example below when the default `8080` port is used: + +``` +> curl -H "Host: printport.default.example.com" +http://$IP +8080 +``` + +## Building + +For details about building and adding new images, see the [section about test +images](/test/README.md#test-images). + diff --git a/test/test_images/printport/main.go b/test/test_images/printport/main.go new file mode 100644 index 000000000000..524d90e8f6af --- /dev/null +++ b/test/test_images/printport/main.go @@ -0,0 +1,43 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "flag" + "fmt" + "log" + "net/http" + "os" +) + +func handler(w http.ResponseWriter, r *http.Request) { + log.Print("PrintPort received a request.") + fmt.Fprintln(w, os.Getenv("PORT")) +} + +func main() { + flag.Parse() + log.Print("PrintPort app started.") + + port, isSet := os.LookupEnv("PORT") + if !isSet { + log.Fatalln("Environment variable PORT is not set.") + } + + http.HandleFunc("/", handler) + http.ListenAndServe(fmt.Sprintf(":%s", port), nil) +} diff --git a/test/test_images/printport/printport.yaml b/test/test_images/printport/printport.yaml new file mode 100644 index 000000000000..cb7992345f89 --- /dev/null +++ b/test/test_images/printport/printport.yaml @@ -0,0 +1,47 @@ +# Copyright 2018 The Knative Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# https://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +apiVersion: serving.knative.dev/v1alpha1 +kind: Configuration +metadata: + name: configuration-example + namespace: default +spec: + revisionTemplate: + metadata: + labels: + knative.dev/type: app + spec: + container: + # This is the Go import path for the binary to containerize + # and substitute here. + image: github.com/knative/serving/test_images/printport + ports: + # Test user-defined port + - containerPort: 8888 + readinessProbe: + httpGet: + path: / + initialDelaySeconds: 3 + periodSeconds: 3 +--- +apiVersion: serving.knative.dev/v1alpha1 +kind: Route +metadata: + name: route-example + namespace: default +spec: + traffic: + - configurationName: configuration-example + percent: 100