From e5b5c56313aefd65c0d5b6960f192665143aa6e7 Mon Sep 17 00:00:00 2001 From: l00388569 Date: Sat, 29 Sep 2018 17:22:59 +0800 Subject: [PATCH 1/8] Support set user-defined port to queue-proxy --- cmd/queue/main.go | 4 +- .../v1alpha1/revision/resources/constants.go | 2 +- .../v1alpha1/revision/resources/deploy.go | 51 ++++--- .../revision/resources/deploy_test.go | 136 +++++++++++++++++- .../v1alpha1/revision/resources/queue.go | 5 +- .../v1alpha1/revision/resources/queue_test.go | 51 ++++++- 6 files changed, 219 insertions(+), 30 deletions(-) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index 7babdf047bf3..cf6a31f49f3a 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -64,6 +64,7 @@ var ( servingRevisionKey string servingAutoscaler string servingAutoscalerPort string + userTargetPort string statChan = make(chan *autoscaler.Stat, statReportingQueueLength) reqChan = make(chan queue.ReqEvent, requestCountingQueueLength) statSink *websocket.ManagedConnection @@ -84,6 +85,7 @@ func initEnv() { servingRevision = util.GetRequiredEnvOrFatal("SERVING_REVISION", logger) servingAutoscaler = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER", logger) servingAutoscalerPort = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER_PORT", logger) + userTargetPort = util.GetRequiredEnvOrFatal("PORT", logger) // TODO(mattmoor): Move this key to be in terms of the KPA. servingRevisionKey = autoscaler.NewKpaKey(servingNamespace, servingRevision) @@ -225,7 +227,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: %s", userTargetPort)) if err != nil { logger.Fatal("Failed to parse localhost url", zap.Error(err)) } diff --git a/pkg/reconciler/v1alpha1/revision/resources/constants.go b/pkg/reconciler/v1alpha1/revision/resources/constants.go index e9617b804f0a..31b9e7dd1bb6 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/constants.go +++ b/pkg/reconciler/v1alpha1/revision/resources/constants.go @@ -31,7 +31,7 @@ const ( IstioOutboundIPRangeAnnotation = "traffic.sidecar.istio.io/includeOutboundIPRanges" userPortName = "user-port" - userPort = 8080 + defaultUserPort = 8080 userPortEnvName = "PORT" autoscalerPort = 8080 diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go index 2008d487a2a4..3f52bbe49c33 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, @@ -81,7 +70,7 @@ var ( } ) -func rewriteUserProbe(p *corev1.Probe) { +func rewriteUserProbe(p *corev1.Probe, userPort int) { if p == nil { return } @@ -101,10 +90,19 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab // update the validations in pkg/webhook.validateContainer. userContainer.Name = userContainerName userContainer.Resources = userResources - userContainer.Ports = userPorts userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) userContainer.Lifecycle = userLifecycle - userContainer.Env = append(userContainer.Env, userEnv) + + userPort, bIsFind := getUserPort(userContainer.Ports) + if !bIsFind { + userPort = &corev1.ContainerPort{ + Name: userPortName, + ContainerPort: int32(defaultUserPort), + } + userContainer.Ports = append(userContainer.Ports, *userPort) + } + userPortEnv := getUserPortEnv(userPort) + userContainer.Env = append(userContainer.Env, userPortEnv) userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...) // Prefer imageDigest from revision if available if rev.Status.ImageDigest != "" { @@ -112,13 +110,13 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab } // If the client provides probes, we should fill in the port for them. - rewriteUserProbe(userContainer.ReadinessProbe) - rewriteUserProbe(userContainer.LivenessProbe) + rewriteUserProbe(userContainer.ReadinessProbe, int(userPort.ContainerPort)) + rewriteUserProbe(userContainer.LivenessProbe, int(userPort.ContainerPort)) podSpec := &corev1.PodSpec{ Containers: []corev1.Container{ *userContainer, - *makeQueueContainer(rev, loggingConfig, autoscalerConfig, controllerConfig), + *makeQueueContainer(rev, loggingConfig, autoscalerConfig, controllerConfig, &userPortEnv), }, Volumes: []corev1.Volume{varLogVolume}, ServiceAccountName: rev.Spec.ServiceAccountName, @@ -133,6 +131,25 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab return podSpec } +func getUserPort(ports []corev1.ContainerPort) (*corev1.ContainerPort, bool) { + for _, port := range ports { + if port.Name == userPortName { + return &port, true + } + } + + return nil, false +} + +func getUserPortEnv(userPort *corev1.ContainerPort) corev1.EnvVar { + // Expose containerPort as env PORT. + userPortEnv := corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(int(userPort.ContainerPort)), + } + return userPortEnv +} + 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 7eb6efd59fa2..6d2f0efbc09f 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go @@ -33,10 +33,21 @@ import ( "github.com/knative/serving/pkg/autoscaler" "github.com/knative/serving/pkg/queue" "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" + "strconv" ) var ( - one int32 = 1 + one int32 = 1 + userPorts = []corev1.ContainerPort{{ + Name: userPortName, + ContainerPort: int32(defaultUserPort), + }} + + // Expose containerPort as env PORT. + userEnv = corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(defaultUserPort), + } ) func TestMakePodSpec(t *testing.T) { @@ -50,6 +61,101 @@ 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, + Container: corev1.Container{ + Image: "busybox", + Ports: []corev1.ContainerPort{ + { + Name: userPortName, + 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: 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, + // These changed based on the Revision and configs passed in. + Args: []string{"-concurrencyQuantumOfTime=0s", "-containerConcurrency=1"}, + 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: "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: "PORT", + Value: "8888", // match user port + }}, + }}, + Volumes: []corev1.Volume{varLogVolume}, + }, + }, { name: "simple concurrency=single no owner", rev: &v1alpha1.Revision{ ObjectMeta: metav1.ObjectMeta{ @@ -202,6 +308,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -286,6 +395,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -306,7 +418,7 @@ func TestMakePodSpec(t *testing.T) { ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ - Port: intstr.FromInt(userPort), + Port: intstr.FromInt(defaultUserPort), Path: "/", }, }, @@ -379,6 +491,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -470,6 +585,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -563,6 +681,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -599,7 +720,7 @@ func TestMakePodSpec(t *testing.T) { LivenessProbe: &corev1.Probe{ Handler: corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(userPort), + Port: intstr.FromInt(defaultUserPort), }, }, }, @@ -652,6 +773,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, @@ -732,6 +856,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: "8080", }}, }, { Name: fluentdContainerName, @@ -864,6 +991,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "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 5f642c743945..19ebd6ef85d7 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue.go @@ -71,7 +71,7 @@ var ( // makeQueueContainer creates the container spec for queue sidecar. func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, autoscalerConfig *autoscaler.Config, - controllerConfig *config.Controller) *corev1.Container { + controllerConfig *config.Controller, userPortEnv *corev1.EnvVar) *corev1.Container { configName := "" if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" { configName = owner.Name @@ -122,6 +122,9 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a }, { Name: "SERVING_LOGGING_LEVEL", Value: loggingLevel, + }, { + Name: "PORT", + Value: userPortEnv.Value, }}, } } diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go index 1cfe3624b9c4..56293e1c38e7 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" "github.com/google/go-cmp/cmp" @@ -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.EnvVar + want *corev1.Container }{{ name: "no owner no autoscaler single", rev: &v1alpha1.Revision{ @@ -57,6 +59,10 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(defaultUserPort), + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -92,6 +98,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: strconv.Itoa(defaultUserPort), }}, }, }, { @@ -111,6 +120,10 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{ QueueSidecarImage: "alpine", }, + userport: &corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(defaultUserPort), + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -147,6 +160,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: strconv.Itoa(defaultUserPort), }}, }, }, { @@ -171,6 +187,10 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(defaultUserPort), + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -206,6 +226,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: strconv.Itoa(defaultUserPort), }}, }, }, { @@ -228,6 +251,10 @@ func TestMakeQueueContainer(t *testing.T) { }, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(defaultUserPort), + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -263,6 +290,9 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", Value: "error", // from logging config + }, { + Name: "PORT", + Value: strconv.Itoa(defaultUserPort), }}, }, }, { @@ -280,6 +310,10 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, + userport: &corev1.EnvVar{ + Name: userPortEnvName, + Value: strconv.Itoa(defaultUserPort), + }, want: &corev1.Container{ // These are effectively constant Name: queueContainerName, @@ -315,13 +349,16 @@ func TestMakeQueueContainer(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "PORT", + Value: strconv.Itoa(defaultUserPort), }}, }, }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := makeQueueContainer(test.rev, test.lc, test.ac, test.cc) + got := makeQueueContainer(test.rev, test.lc, test.ac, test.cc, test.userport) if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("makeQueueContainer (-want, +got) = %v", diff) } From 064bf3ce32fb0c45ba03159359f57c1fe66df1ec Mon Sep 17 00:00:00 2001 From: l00388569 Date: Sun, 14 Oct 2018 10:25:19 +0800 Subject: [PATCH 2/8] change queue-proxy user-port env name --- cmd/queue/main.go | 2 +- .../v1alpha1/revision/resources/deploy.go | 21 +++++++++--- .../revision/resources/deploy_test.go | 18 +++++----- .../v1alpha1/revision/resources/queue.go | 10 +++--- .../v1alpha1/revision/resources/queue_test.go | 34 +++++++++---------- 5 files changed, 48 insertions(+), 37 deletions(-) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index cf6a31f49f3a..4db4c28d9ec7 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -85,7 +85,7 @@ func initEnv() { servingRevision = util.GetRequiredEnvOrFatal("SERVING_REVISION", logger) servingAutoscaler = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER", logger) servingAutoscalerPort = util.GetRequiredEnvOrFatal("SERVING_AUTOSCALER_PORT", logger) - userTargetPort = util.GetRequiredEnvOrFatal("PORT", logger) + userTargetPort = util.GetRequiredEnvOrFatal("USER_PORT", logger) // TODO(mattmoor): Move this key to be in terms of the KPA. servingRevisionKey = autoscaler.NewKpaKey(servingNamespace, servingRevision) diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go index 3f52bbe49c33..a29548bf17da 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy.go @@ -93,15 +93,15 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) userContainer.Lifecycle = userLifecycle - userPort, bIsFind := getUserPort(userContainer.Ports) - if !bIsFind { + userPort, found := getUserPort(userContainer.Ports) + if !found { userPort = &corev1.ContainerPort{ Name: userPortName, ContainerPort: int32(defaultUserPort), } userContainer.Ports = append(userContainer.Ports, *userPort) } - userPortEnv := getUserPortEnv(userPort) + userPortEnv := buildUserPortEnv(userPort) userContainer.Env = append(userContainer.Env, userPortEnv) userContainer.Env = append(userContainer.Env, getKnativeEnvVar(rev)...) // Prefer imageDigest from revision if available @@ -116,7 +116,7 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab podSpec := &corev1.PodSpec{ Containers: []corev1.Container{ *userContainer, - *makeQueueContainer(rev, loggingConfig, autoscalerConfig, controllerConfig, &userPortEnv), + *makeQueueContainer(rev, loggingConfig, autoscalerConfig, controllerConfig, buildQueueProxyExtraEnv(userPort)), }, Volumes: []corev1.Volume{varLogVolume}, ServiceAccountName: rev.Spec.ServiceAccountName, @@ -141,7 +141,7 @@ func getUserPort(ports []corev1.ContainerPort) (*corev1.ContainerPort, bool) { return nil, false } -func getUserPortEnv(userPort *corev1.ContainerPort) corev1.EnvVar { +func buildUserPortEnv(userPort *corev1.ContainerPort) corev1.EnvVar { // Expose containerPort as env PORT. userPortEnv := corev1.EnvVar{ Name: userPortEnvName, @@ -150,6 +150,17 @@ func getUserPortEnv(userPort *corev1.ContainerPort) corev1.EnvVar { return userPortEnv } +func buildQueueProxyExtraEnv(userPort *corev1.ContainerPort) []corev1.EnvVar { + queueContainerExtraEnv := []corev1.EnvVar{ + { + Name: "USER_PORT", + Value: strconv.Itoa(int(userPort.ContainerPort)), + }, + } + + return queueContainerExtraEnv +} + 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 6d2f0efbc09f..0bdf2886cc11 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go @@ -149,7 +149,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8888", // match user port }}, }}, @@ -309,7 +309,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, @@ -396,7 +396,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, @@ -492,7 +492,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, @@ -586,7 +586,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, @@ -682,7 +682,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, @@ -774,7 +774,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, @@ -857,7 +857,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }, { @@ -992,7 +992,7 @@ func TestMakePodSpec(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: "8080", }}, }}, diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue.go b/pkg/reconciler/v1alpha1/revision/resources/queue.go index 19ebd6ef85d7..ebbda5a2f0bf 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue.go @@ -71,7 +71,7 @@ var ( // makeQueueContainer creates the container spec for queue sidecar. func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, autoscalerConfig *autoscaler.Config, - controllerConfig *config.Controller, userPortEnv *corev1.EnvVar) *corev1.Container { + controllerConfig *config.Controller, extraEnvs []corev1.EnvVar) *corev1.Container { configName := "" if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" { configName = owner.Name @@ -84,7 +84,7 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a loggingLevel = ll.String() } - return &corev1.Container{ + queueContainer := &corev1.Container{ Name: queueContainerName, Image: controllerConfig.QueueSidecarImage, Resources: queueResources, @@ -122,9 +122,9 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a }, { Name: "SERVING_LOGGING_LEVEL", Value: loggingLevel, - }, { - Name: "PORT", - Value: userPortEnv.Value, }}, } + + queueContainer.Env = append(queueContainer.Env, extraEnvs...) + return queueContainer } diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go index 56293e1c38e7..167ad94d053d 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go @@ -42,7 +42,7 @@ func TestMakeQueueContainer(t *testing.T) { lc *logging.Config ac *autoscaler.Config cc *config.Controller - userport *corev1.EnvVar + userport *corev1.ContainerPort want *corev1.Container }{{ name: "no owner no autoscaler single", @@ -59,9 +59,9 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, - userport: &corev1.EnvVar{ + userport: &corev1.ContainerPort{ Name: userPortEnvName, - Value: strconv.Itoa(defaultUserPort), + ContainerPort: defaultUserPort, }, want: &corev1.Container{ // These are effectively constant @@ -99,7 +99,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -120,9 +120,9 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{ QueueSidecarImage: "alpine", }, - userport: &corev1.EnvVar{ + userport: &corev1.ContainerPort{ Name: userPortEnvName, - Value: strconv.Itoa(defaultUserPort), + ContainerPort: defaultUserPort, }, want: &corev1.Container{ // These are effectively constant @@ -161,7 +161,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -187,9 +187,9 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, - userport: &corev1.EnvVar{ + userport: &corev1.ContainerPort{ Name: userPortEnvName, - Value: strconv.Itoa(defaultUserPort), + ContainerPort: defaultUserPort, }, want: &corev1.Container{ // These are effectively constant @@ -227,7 +227,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -251,9 +251,9 @@ func TestMakeQueueContainer(t *testing.T) { }, ac: &autoscaler.Config{}, cc: &config.Controller{}, - userport: &corev1.EnvVar{ + userport: &corev1.ContainerPort{ Name: userPortEnvName, - Value: strconv.Itoa(defaultUserPort), + ContainerPort: defaultUserPort, }, want: &corev1.Container{ // These are effectively constant @@ -291,7 +291,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", Value: "error", // from logging config }, { - Name: "PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -310,9 +310,9 @@ func TestMakeQueueContainer(t *testing.T) { lc: &logging.Config{}, ac: &autoscaler.Config{}, cc: &config.Controller{}, - userport: &corev1.EnvVar{ + userport: &corev1.ContainerPort{ Name: userPortEnvName, - Value: strconv.Itoa(defaultUserPort), + ContainerPort: defaultUserPort, }, want: &corev1.Container{ // These are effectively constant @@ -350,7 +350,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -358,7 +358,7 @@ func TestMakeQueueContainer(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := makeQueueContainer(test.rev, test.lc, test.ac, test.cc, test.userport) + got := makeQueueContainer(test.rev, test.lc, test.ac, test.cc, buildQueueProxyExtraEnv(test.userport)) if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("makeQueueContainer (-want, +got) = %v", diff) } From 5fe7fb89d56c4bfb1c9c63923ba247999c0658ef Mon Sep 17 00:00:00 2001 From: l00388569 Date: Mon, 15 Oct 2018 11:00:59 +0800 Subject: [PATCH 3/8] rebase master, fix uint test --- cmd/queue/main.go | 2 +- pkg/reconciler/v1alpha1/revision/resources/deploy_test.go | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/cmd/queue/main.go b/cmd/queue/main.go index 4db4c28d9ec7..56f756b07ec0 100644 --- a/cmd/queue/main.go +++ b/cmd/queue/main.go @@ -227,7 +227,7 @@ func main() { zap.String(logkey.Key, servingRevisionKey), zap.String(logkey.Pod, podName)) - target, err := url.Parse(fmt.Sprintf("http://localhost: %s", userTargetPort)) + target, err := url.Parse(fmt.Sprintf("http://localhost:%s", userTargetPort)) if err != nil { logger.Fatal("Failed to parse localhost url", zap.Error(err)) } diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go index 0bdf2886cc11..57e02d1aa7d8 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go @@ -88,7 +88,7 @@ func TestMakePodSpec(t *testing.T) { cc: &config.Controller{}, want: &corev1.PodSpec{ Containers: []corev1.Container{{ - Name: UserContainerName, + Name: userContainerName, Image: "busybox", Resources: userResources, Ports: []corev1.ContainerPort{ @@ -121,7 +121,7 @@ func TestMakePodSpec(t *testing.T) { Lifecycle: queueLifecycle, ReadinessProbe: queueReadinessProbe, // These changed based on the Revision and configs passed in. - Args: []string{"-concurrencyQuantumOfTime=0s", "-containerConcurrency=1"}, + Args: []string{"-containerConcurrency=1"}, Env: []corev1.EnvVar{{ Name: "SERVING_NAMESPACE", Value: "foo", // matches namespace @@ -228,6 +228,9 @@ func TestMakePodSpec(t *testing.T) { }, { Name: "SERVING_LOGGING_LEVEL", // No logging level + }, { + Name: "USER_PORT", + Value: "8080", }}, }}, Volumes: []corev1.Volume{varLogVolume}, From 7fabfe67cafda5e14ad9575e979a77d04bd01a0a Mon Sep 17 00:00:00 2001 From: leonlm Date: Tue, 30 Oct 2018 09:16:41 +0800 Subject: [PATCH 4/8] remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false --- config/activator.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/activator.yaml b/config/activator.yaml index 87c19dcfdb90..55e0ed77ddfb 100644 --- a/config/activator.yaml +++ b/config/activator.yaml @@ -26,7 +26,7 @@ spec: template: metadata: annotations: - sidecar.istio.io/inject: "true" + sidecar.istio.io/inject: "false" labels: app: activator role: activator From 4a190994fd1208de2a65ecadd45afb9a133367bc Mon Sep 17 00:00:00 2001 From: leonlm Date: Fri, 26 Oct 2018 16:32:33 +0800 Subject: [PATCH 5/8] add e2e test for user-port && review comments modify && open acitvator's istio-proxy --- config/activator.yaml | 2 +- .../serving/v1alpha1/revision_validation.go | 15 ++- .../v1alpha1/revision/resources/deploy.go | 47 +++---- .../v1alpha1/revision/resources/queue.go | 8 +- .../v1alpha1/revision/resources/queue_test.go | 22 ++-- test/configuration.go | 1 + test/conformance/service_test.go | 121 ++++++++++++++++++ test/crd.go | 4 + test/e2e/autoscale_test.go | 30 ++++- test/states.go | 9 +- test/test_images/autoscale-userport/README.md | 12 ++ .../autoscale-userport/autoscale.go | 104 +++++++++++++++ 12 files changed, 333 insertions(+), 42 deletions(-) create mode 100644 test/test_images/autoscale-userport/README.md create mode 100644 test/test_images/autoscale-userport/autoscale.go diff --git a/config/activator.yaml b/config/activator.yaml index 55e0ed77ddfb..87c19dcfdb90 100644 --- a/config/activator.yaml +++ b/config/activator.yaml @@ -26,7 +26,7 @@ spec: template: metadata: annotations: - sidecar.istio.io/inject: "false" + sidecar.istio.io/inject: "true" labels: app: activator role: activator diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go index 882682d939d9..619173ee3f69 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation.go +++ b/pkg/apis/serving/v1alpha1/revision_validation.go @@ -110,7 +110,9 @@ func validateContainer(container corev1.Container) *apis.FieldError { ignoredFields = append(ignoredFields, "resources") } if len(container.Ports) > 0 { - ignoredFields = append(ignoredFields, "ports") + if !validateContainerPorts(container.Ports) { + ignoredFields = append(ignoredFields, "ports") + } } if len(container.VolumeMounts) > 0 { ignoredFields = append(ignoredFields, "volumeMounts") @@ -133,6 +135,17 @@ func validateContainer(container corev1.Container) *apis.FieldError { return errs } +func validateContainerPorts(ports []corev1.ContainerPort) bool { + bIsAllowed := false + // users can set container port which names "user-port" to define application's port. + // Queue-proxy will use it to send requests to application + if len(ports) == 1 && ports[0].Name == "user-port"{ + bIsAllowed = true + return bIsAllowed + } + return bIsAllowed +} + func validateBuildRef(buildRef *corev1.ObjectReference) *apis.FieldError { if buildRef == nil { return nil diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go index a29548bf17da..bea3efc053cb 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy.go @@ -93,30 +93,26 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab userContainer.VolumeMounts = append(userContainer.VolumeMounts, varLogVolumeMount) userContainer.Lifecycle = userLifecycle - userPort, found := getUserPort(userContainer.Ports) + userPort, found := getUserPort(rev) if !found { - userPort = &corev1.ContainerPort{ - Name: userPortName, - ContainerPort: int32(defaultUserPort), - } - userContainer.Ports = append(userContainer.Ports, *userPort) + createAndSetDefaultUserPort(userContainer) } - userPortEnv := buildUserPortEnv(userPort) - userContainer.Env = append(userContainer.Env, userPortEnv) + userContainer.Env = append(userContainer.Env, buildUserPortEnv(userPort)) 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, int(userPort.ContainerPort)) - rewriteUserProbe(userContainer.LivenessProbe, int(userPort.ContainerPort)) + rewriteUserProbe(userContainer.ReadinessProbe, int(userPort)) + rewriteUserProbe(userContainer.LivenessProbe, int(userPort)) podSpec := &corev1.PodSpec{ Containers: []corev1.Container{ *userContainer, - *makeQueueContainer(rev, loggingConfig, autoscalerConfig, controllerConfig, buildQueueProxyExtraEnv(userPort)), + *makeQueueContainer(rev, loggingConfig, autoscalerConfig, controllerConfig), }, Volumes: []corev1.Volume{varLogVolume}, ServiceAccountName: rev.Spec.ServiceAccountName, @@ -131,31 +127,38 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab return podSpec } -func getUserPort(ports []corev1.ContainerPort) (*corev1.ContainerPort, bool) { - for _, port := range ports { +func createAndSetDefaultUserPort(userContainer *corev1.Container) { + defaultUserPort := &corev1.ContainerPort{ + Name: userPortName, + ContainerPort: int32(defaultUserPort), + } + userContainer.Ports = append(userContainer.Ports, *defaultUserPort) +} + +func getUserPort(rev *v1alpha1.Revision) (int32, bool) { + for _, port := range rev.Spec.Container.Ports { if port.Name == userPortName { - return &port, true + return port.ContainerPort, true } } - return nil, false + return defaultUserPort, false } -func buildUserPortEnv(userPort *corev1.ContainerPort) corev1.EnvVar { +func buildUserPortEnv(userPort int32) corev1.EnvVar { // Expose containerPort as env PORT. userPortEnv := corev1.EnvVar{ Name: userPortEnvName, - Value: strconv.Itoa(int(userPort.ContainerPort)), + Value: strconv.Itoa(int(userPort)), } return userPortEnv } func buildQueueProxyExtraEnv(userPort *corev1.ContainerPort) []corev1.EnvVar { - queueContainerExtraEnv := []corev1.EnvVar{ - { - Name: "USER_PORT", - Value: strconv.Itoa(int(userPort.ContainerPort)), - }, + queueContainerExtraEnv := []corev1.EnvVar{{ + Name: "USER_PORT", + Value: strconv.Itoa(int(userPort.ContainerPort)), + }, } return queueContainerExtraEnv diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue.go b/pkg/reconciler/v1alpha1/revision/resources/queue.go index ebbda5a2f0bf..b8ba3086357f 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue.go @@ -71,13 +71,14 @@ var ( // makeQueueContainer creates the container spec for queue sidecar. func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, autoscalerConfig *autoscaler.Config, - controllerConfig *config.Controller, extraEnvs []corev1.EnvVar) *corev1.Container { + controllerConfig *config.Controller) *corev1.Container { configName := "" if owner := metav1.GetControllerOf(rev); owner != nil && owner.Kind == "Configuration" { configName = owner.Name } autoscalerAddress := "autoscaler" + userport, _ := getUserPort(rev) var loggingLevel string if ll, ok := loggingConfig.LoggingLevel["queueproxy"]; ok { @@ -122,9 +123,10 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a }, { Name: "SERVING_LOGGING_LEVEL", Value: loggingLevel, + }, { + Name: "USER_PORT", + Value: strconv.Itoa(int(userport)), }}, } - - queueContainer.Env = append(queueContainer.Env, extraEnvs...) return queueContainer } diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go index 167ad94d053d..6eee444157a1 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go @@ -60,7 +60,7 @@ func TestMakeQueueContainer(t *testing.T) { ac: &autoscaler.Config{}, cc: &config.Controller{}, userport: &corev1.ContainerPort{ - Name: userPortEnvName, + Name: userPortEnvName, ContainerPort: defaultUserPort, }, want: &corev1.Container{ @@ -99,7 +99,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "USER_PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -121,7 +121,7 @@ func TestMakeQueueContainer(t *testing.T) { QueueSidecarImage: "alpine", }, userport: &corev1.ContainerPort{ - Name: userPortEnvName, + Name: userPortEnvName, ContainerPort: defaultUserPort, }, want: &corev1.Container{ @@ -161,7 +161,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "USER_PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -188,7 +188,7 @@ func TestMakeQueueContainer(t *testing.T) { ac: &autoscaler.Config{}, cc: &config.Controller{}, userport: &corev1.ContainerPort{ - Name: userPortEnvName, + Name: userPortEnvName, ContainerPort: defaultUserPort, }, want: &corev1.Container{ @@ -227,7 +227,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "USER_PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -252,7 +252,7 @@ func TestMakeQueueContainer(t *testing.T) { ac: &autoscaler.Config{}, cc: &config.Controller{}, userport: &corev1.ContainerPort{ - Name: userPortEnvName, + Name: userPortEnvName, ContainerPort: defaultUserPort, }, want: &corev1.Container{ @@ -291,7 +291,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", Value: "error", // from logging config }, { - Name: "USER_PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -311,7 +311,7 @@ func TestMakeQueueContainer(t *testing.T) { ac: &autoscaler.Config{}, cc: &config.Controller{}, userport: &corev1.ContainerPort{ - Name: userPortEnvName, + Name: userPortEnvName, ContainerPort: defaultUserPort, }, want: &corev1.Container{ @@ -350,7 +350,7 @@ func TestMakeQueueContainer(t *testing.T) { Name: "SERVING_LOGGING_LEVEL", // No logging level }, { - Name: "USER_PORT", + Name: "USER_PORT", Value: strconv.Itoa(defaultUserPort), }}, }, @@ -358,7 +358,7 @@ func TestMakeQueueContainer(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got := makeQueueContainer(test.rev, test.lc, test.ac, test.cc, buildQueueProxyExtraEnv(test.userport)) + got := makeQueueContainer(test.rev, test.lc, test.ac, test.cc) if diff := cmp.Diff(test.want, got, cmpopts.IgnoreUnexported(resource.Quantity{})); diff != "" { t.Errorf("makeQueueContainer (-want, +got) = %v", diff) } diff --git a/test/configuration.go b/test/configuration.go index 895f15c7a42f..1d210bad7ebe 100644 --- a/test/configuration.go +++ b/test/configuration.go @@ -26,6 +26,7 @@ import ( // Options are test setup parameters. type Options struct { EnvVars []corev1.EnvVar + ContainerPorts []corev1.ContainerPort ContainerConcurrency int } diff --git a/test/conformance/service_test.go b/test/conformance/service_test.go index af89b8b9e61e..e47cd1604843 100644 --- a/test/conformance/service_test.go +++ b/test/conformance/service_test.go @@ -24,6 +24,7 @@ import ( "math" "net/http" "testing" + "time" pkgTest "github.com/knative/pkg/test" "github.com/knative/pkg/test/logging" @@ -31,6 +32,7 @@ import ( serviceresourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/service/resources/names" "github.com/knative/serving/test" "golang.org/x/sync/errgroup" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -38,6 +40,73 @@ const ( expectedLatest = "Hello World! How about some tasty noodles?" ) +func setServiceCustomUserPort(svc *v1alpha1.Service, userPort string) { + container := svc.Spec.RunLatest.Configuration.RevisionTemplate.Spec.Container + // find user-port and set + for index, envItem := range container.Env { + if envItem.Name == "user-port" { + container.Env[index].Value = userPort + return + } + } + portEnv := corev1.EnvVar{ + Name: "user-port", + Value: userPort, + } + container.Env = append(container.Env, portEnv) +} + +func assertServiceUserPortSet(t *testing.T, clients *test.Clients, names test.ResourceNames, assertUserPort string) { + deploymentName := names.Revision + "-deployment" + + err := test.WaitForDeploymentState( + clients.KubeClient, + deploymentName, + test.IsDeploymentReady, + "DeploymentIsScaledUp", + 2*time.Minute) + if err != nil { + t.Fatalf("Unable to observe the Deployment named %s scaling up. %s", deploymentName, err) + } + + deployment, err := clients.KubeClient.Kube.ExtensionsV1beta1().Deployments(test.ServingNamespace).Get(deploymentName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error get revision %v's deployment. err: %v", names.Revision, err) + } + + var userContainerPortEnv *corev1.EnvVar + var queueProxyPortEnv *corev1.EnvVar + + for _, container := range deployment.Spec.Template.Spec.Containers { + if container.Name == "user-container" { + for _, env := range container.Env { + if env.Name == "PORT" { + userContainerPortEnv = &env + } + } + } else if container.Name == "queue-proxy" { + for _, env := range container.Env { + if env.Name == "USER_PORT" { + queueProxyPortEnv = &env + } + } + } + } + if userContainerPortEnv == nil { + t.Fatalf("can't find user container's port env.") + } + if queueProxyPortEnv == nil { + t.Fatalf("can't find queue-proxy container's port env.") + } + + if userContainerPortEnv.Value == assertUserPort { + t.Fatalf("user container's port is not expect. %v", userContainerPortEnv.Value) + } + if queueProxyPortEnv.Value == assertUserPort { + t.Fatalf("queue-proxy container's port is not expect. %v", queueProxyPortEnv.Value) + } +} + func waitForExpected(logger *logging.BaseLogger, clients *test.Clients, domain, expected string) error { client, err := pkgTest.NewSpoofingClient(clients.KubeClient, logger, domain, test.ServingFlags.ResolvableDomain) if err != nil { @@ -406,6 +475,58 @@ func TestReleaseService(t *testing.T) { []string{expectedLatest, expectedGreen, expectedBlue}) } +func TestRunLatestServiceWithUserPort(t *testing.T) { + clients := setup(t) + + // Add test case specific name to its own logger. + logger := logging.GetContextLogger("TestRunLatestService") + + var imagePaths []string + imagePaths = append(imagePaths, test.ImagePath(pizzaPlanet1)) + imagePaths = append(imagePaths, test.ImagePath(pizzaPlanet2)) + + var names test.ResourceNames + names.Service = test.AppendRandomString("pizzaplanet-service", logger) + + defer tearDown(clients, names) + test.CleanupOnInterrupt(func() { tearDown(clients, names) }, logger) + + logger.Info("Creating a new Service") + svc, err := test.CreateLatestService(logger, clients, names, imagePaths[0]) + if err != nil { + t.Fatalf("Failed to create Service: %v", err) + } + + logger.Info("Set user-port to Service") + expectPort := "8866" + setServiceCustomUserPort(svc, expectPort) + + names.Route = serviceresourcenames.Route(svc) + names.Config = serviceresourcenames.Configuration(svc) + + logger.Info("The Service will be updated with the name of the Revision once it is created") + revisionName, err := waitForServiceLatestCreatedRevision(clients, names) + if err != nil { + t.Fatalf("Service %s was not updated with the new revision: %v", names.Service, err) + } + names.Revision = revisionName + + logger.Info("The Service will be updated with the domain of the Route once it is created") + routeDomain, err := waitForServiceDomain(clients, names) + if err != nil { + t.Fatalf("Service %s was not updated with the new route: %v", names.Service, err) + } + + logger.Info("When the Service reports as Ready, everything should be ready.") + if err := test.WaitForServiceState(clients.ServingClient, names.Service, test.IsServiceReady, "ServiceIsReady"); err != nil { + t.Fatalf("The Service %s was not marked as Ready to serve traffic to Revision %s: %v", names.Service, names.Revision, err) + } + assertServiceResourcesUpdated(t, logger, clients, names, routeDomain, "1", "What a spaceport!") + + logger.Info("Check queue-proxy container's user-port Env and user container's PORT Env.") + assertServiceUserPortSet(t, clients, names, expectPort) +} + // TODO(jonjohnsonjr): LatestService roads less traveled. // TODO(jonjohnsonjr): PinnedService happy path. // TODO(jonjohnsonjr): PinnedService roads less traveled. diff --git a/test/crd.go b/test/crd.go index 1427c9acd65b..cfc7ea10fec6 100644 --- a/test/crd.go +++ b/test/crd.go @@ -101,6 +101,7 @@ func Configuration(namespace string, names ResourceNames, imagePath string, opti Spec: v1alpha1.RevisionSpec{ Container: corev1.Container{ Image: imagePath, + ImagePullPolicy: corev1.PullIfNotPresent, }, ContainerConcurrency: v1alpha1.RevisionContainerConcurrencyType(options.ContainerConcurrency), }, @@ -110,6 +111,9 @@ func Configuration(namespace string, names ResourceNames, imagePath string, opti if options.EnvVars != nil && len(options.EnvVars) > 0 { config.Spec.RevisionTemplate.Spec.Container.Env = options.EnvVars } + if options.ContainerPorts != nil && len(options.ContainerPorts) > 0 { + config.Spec.RevisionTemplate.Spec.Container.Ports = options.ContainerPorts + } return config } diff --git a/test/e2e/autoscale_test.go b/test/e2e/autoscale_test.go index 43c7f8c7961f..55e3966c5a1b 100644 --- a/test/e2e/autoscale_test.go +++ b/test/e2e/autoscale_test.go @@ -133,7 +133,7 @@ type testContext struct { domain string } -func setup(t *testing.T) *testContext { +func setup(t *testing.T, userport int32, imageName string) *testContext { //add test case specific name to its own logger logger := logging.GetContextLogger(t.Name()) clients := Setup(t) @@ -147,11 +147,12 @@ func setup(t *testing.T) *testContext { t.Fatalf("Unable to parse scale-to-zero-threshold as duration: %v", err) } - imagePath := test.ImagePath("autoscale") + imagePath := test.ImagePath(imageName) logger.Infof("Creating a new Route and Configuration") names, err := CreateRouteAndConfig(clients, logger, imagePath, &test.Options{ ContainerConcurrency: 10, + ContainerPorts: generateUserPort(logger, userport), }) if err != nil { t.Fatalf("Failed to create Route and Configuration: %v", err) @@ -205,6 +206,18 @@ func setup(t *testing.T) *testContext { } } +func generateUserPort(logger *logging.BaseLogger, port int32) []v1.ContainerPort { + containerPorts := []v1.ContainerPort{} + userPort := v1.ContainerPort{ + Name: "user-port", + ContainerPort: int32(port), + } + logger.Infof("set user port: %v", port) + + containerPorts = append(containerPorts, userPort) + return containerPorts +} + func assertScaleUp(ctx *testContext) { ctx.logger.Infof("The autoscaler spins up additional replicas when traffic increases.") err := generateTraffic(ctx, 20, 20*time.Second) @@ -259,7 +272,18 @@ func assertScaleDown(ctx *testContext) { } func TestAutoscaleUpDownUp(t *testing.T) { - ctx := setup(t) + ctx := setup(t, 8080, "autoscale") + stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger) + defer close(stopChan) + defer tearDown(ctx) + + assertScaleUp(ctx) + assertScaleDown(ctx) + assertScaleUp(ctx) +} + +func TestAutoscaleUpDownUpUserPort(t *testing.T) { + ctx := setup(t, 8888, "autoscale-userport") stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger) defer close(stopChan) defer tearDown(ctx) diff --git a/test/states.go b/test/states.go index cc1d3882e2aa..fe86cde6b049 100644 --- a/test/states.go +++ b/test/states.go @@ -18,8 +18,8 @@ package test import ( "fmt" - corev1 "k8s.io/api/core/v1" + apiv1beta1 "k8s.io/api/extensions/v1beta1" "github.com/knative/serving/pkg/apis/serving" "github.com/knative/serving/pkg/apis/serving/v1alpha1" @@ -97,6 +97,13 @@ func IsRouteReady(r *v1alpha1.Route) (bool, error) { return r.Status.IsReady(), nil } +func IsDeploymentReady(d *apiv1beta1.Deployment) (bool, error) { + return d.Status.UpdatedReplicas == *(d.Spec.Replicas) && + d.Status.Replicas == *(d.Spec.Replicas) && + d.Status.AvailableReplicas == *(d.Spec.Replicas) && + d.Status.ObservedGeneration >= d.Generation, nil +} + // ConfigurationHasCreatedRevision returns whether the Configuration has created a Revision. func ConfigurationHasCreatedRevision(c *v1alpha1.Configuration) (bool, error) { return c.Status.LatestCreatedRevisionName != "", nil diff --git a/test/test_images/autoscale-userport/README.md b/test/test_images/autoscale-userport/README.md new file mode 100644 index 000000000000..a573906fceb0 --- /dev/null +++ b/test/test_images/autoscale-userport/README.md @@ -0,0 +1,12 @@ +# Autoscale test image + +This directory contains the test image used in the autoscale e2e tests. + +The image contains a simple Go webserver, `autoscale.go`, that will, by default, listen on port `8888` and expose a service at `/`. + +When called, the server calculates the highest prime less than 40000000 and emits a message. + +## 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/autoscale-userport/autoscale.go b/test/test_images/autoscale-userport/autoscale.go new file mode 100644 index 000000000000..990f4429f808 --- /dev/null +++ b/test/test_images/autoscale-userport/autoscale.go @@ -0,0 +1,104 @@ +/* +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 + + http://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 ( + "fmt" + "log" + "math" + "net/http" + "sync" + "time" +) + +// Algorithm from https://stackoverflow.com/a/21854246 + +// Only primes less than or equal to N will be generated +func primes(N int) []int { + + var x, y, n int + nsqrt := math.Sqrt(float64(N)) + + isPrime := make([]bool, N) + + for x = 1; float64(x) <= nsqrt; x++ { + for y = 1; float64(y) <= nsqrt; y++ { + n = 4*(x*x) + y*y + if n <= N && (n%12 == 1 || n%12 == 5) { + isPrime[n] = !isPrime[n] + } + n = 3*(x*x) + y*y + if n <= N && n%12 == 7 { + isPrime[n] = !isPrime[n] + } + n = 3*(x*x) - y*y + if x > y && n <= N && n%12 == 11 { + isPrime[n] = !isPrime[n] + } + } + } + + for n = 5; float64(n) <= nsqrt; n++ { + if isPrime[n] { + for y = n * n; y < N; y += n * n { + isPrime[y] = false + } + } + } + + isPrime[2] = true + isPrime[3] = true + + primes := make([]int, 0, 1270606) + for x = 0; x < len(isPrime)-1; x++ { + if isPrime[x] { + primes = append(primes, x) + } + } + + // primes is now a slice that contains all primes numbers up to N + return primes +} + +func handler(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + p := primes(400000) + largest := p[len(p)-1] + msg := fmt.Sprintf("The largest prime under 400000 is %d. Enjoy your noodles!", largest) + fmt.Fprintf(w, msg) + log.Printf(msg) + }() + go func() { + defer wg.Done() + start := time.Now() + time.Sleep(time.Second) + msg := fmt.Sprintf("Slept for %v.", time.Since(start)) + fmt.Fprintf(w, msg) + log.Printf(msg) + }() + wg.Wait() +} + +func main() { + http.HandleFunc("/", handler) + http.ListenAndServe(":8888", nil) +} From 98428ad42697d239ca74c0c99b90994c9daecb78 Mon Sep 17 00:00:00 2001 From: leonlm Date: Thu, 1 Nov 2018 15:18:14 +0800 Subject: [PATCH 6/8] set more validation info about user-port && modify e2e test --- pkg/apis/serving/v1alpha1/revision_types.go | 5 + .../serving/v1alpha1/revision_validation.go | 34 +++-- .../v1alpha1/revision_validation_test.go | 30 ++++- .../v1alpha1/revision/resources/constants.go | 2 - .../v1alpha1/revision/resources/deploy.go | 18 +-- .../revision/resources/deploy_test.go | 14 +-- .../v1alpha1/revision/resources/queue_test.go | 20 +-- test/e2e/autoscale_test.go | 32 +---- test/e2e/helloworld_test.go | 118 ++++++++++++++++++ test/test_images/autoscale-userport/README.md | 12 -- .../autoscale-userport/autoscale.go | 104 --------------- .../test_images/helloworld-userport/README.md | 15 +++ .../helloworld-userport/helloworld.go | 37 ++++++ .../helloworld-userport/helloworld.yaml | 44 +++++++ 14 files changed, 293 insertions(+), 192 deletions(-) delete mode 100644 test/test_images/autoscale-userport/README.md delete mode 100644 test/test_images/autoscale-userport/autoscale.go create mode 100644 test/test_images/helloworld-userport/README.md create mode 100644 test/test_images/helloworld-userport/helloworld.go create mode 100644 test/test_images/helloworld-userport/helloworld.yaml diff --git a/pkg/apis/serving/v1alpha1/revision_types.go b/pkg/apis/serving/v1alpha1/revision_types.go index 2cc729740487..fb42f7930e02 100644 --- a/pkg/apis/serving/v1alpha1/revision_types.go +++ b/pkg/apis/serving/v1alpha1/revision_types.go @@ -119,6 +119,11 @@ const ( RevisionContainerConcurrencyMax RevisionContainerConcurrencyType = 1000 ) +const ( + RevisionContainerUserPortName = "user-port" + RevisionContainerUserPortDefaultValue = 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 619173ee3f69..52fb17fc974a 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation.go +++ b/pkg/apis/serving/v1alpha1/revision_validation.go @@ -17,6 +17,7 @@ limitations under the License. package v1alpha1 import ( + "fmt" "strconv" "github.com/google/go-cmp/cmp" @@ -109,11 +110,6 @@ func validateContainer(container corev1.Container) *apis.FieldError { if !equality.Semantic.DeepEqual(container.Resources, corev1.ResourceRequirements{}) { ignoredFields = append(ignoredFields, "resources") } - if len(container.Ports) > 0 { - if !validateContainerPorts(container.Ports) { - ignoredFields = append(ignoredFields, "ports") - } - } if len(container.VolumeMounts) > 0 { ignoredFields = append(ignoredFields, "volumeMounts") } @@ -125,6 +121,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) + } // Validate our probes if err := validateProbe(container.ReadinessProbe).ViaField("readinessProbe"); err != nil { errs = errs.Also(err) @@ -135,15 +134,26 @@ func validateContainer(container corev1.Container) *apis.FieldError { return errs } -func validateContainerPorts(ports []corev1.ContainerPort) bool { - bIsAllowed := false - // users can set container port which names "user-port" to define application's port. +func validateContainerPorts(ports []corev1.ContainerPort) *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 len(ports) == 1 && ports[0].Name == "user-port"{ - bIsAllowed = true - return bIsAllowed + // if user didn't set any port, it will set default port user-port=8080. + if len(ports) > 1 { + return &apis.FieldError{ + Message: "container ports set more than one", + Paths: []string{"ports"}, + Details: "only can be set named \"user-port\" port", + } } - return bIsAllowed + + if len(ports) == 1 && ports[0].Name != "user-port"{ + return &apis.FieldError{ + Message: fmt.Sprintf("unsupport port name %v", ports[0].Name), + Paths: []string{"ports"}, + Details: "only can be set named \"user-port\" port", + } + } + return nil } func validateBuildRef(buildRef *corev1.ObjectReference) *apis.FieldError { diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index d559d817aa30..899718023933 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -64,14 +64,34 @@ func TestContainerValidation(t *testing.T) { }, want: apis.ErrDisallowedFields("resources"), }, { - name: "has ports", + name: "has more than one port", c: corev1.Container{ Ports: []corev1.ContainerPort{{ Name: "http", ContainerPort: 8080, + },{ + Name: "http2", + ContainerPort: 8181, }}, }, - want: apis.ErrDisallowedFields("ports"), + want: &apis.FieldError{ + Message: "container ports set more than one", + Paths: []string{"ports"}, + Details: "only can be set named \"user-port\" port", + }, + }, { + name: "set wrong name port", + c: corev1.Container{ + Ports: []corev1.ContainerPort{{ + Name: "http", + ContainerPort: 8080, + }}, + }, + want: &apis.FieldError{ + Message: fmt.Sprintf("unsupport port name %v", "http"), + Paths: []string{"ports"}, + Details: "only can be set named \"user-port\" port", + }, }, { name: "has volumeMounts", c: corev1.Container{ @@ -151,7 +171,11 @@ func TestContainerValidation(t *testing.T) { }}, Lifecycle: &corev1.Lifecycle{}, }, - want: apis.ErrDisallowedFields("name", "resources", "ports", "volumeMounts", "lifecycle"), + want: apis.ErrDisallowedFields("name", "resources", "volumeMounts", "lifecycle").Also(&apis.FieldError{ + Message: fmt.Sprintf("unsupport port name %v", "http"), + Paths: []string{"ports"}, + Details: "only can be set named \"user-port\" port", + }), }} for _, test := range tests { diff --git a/pkg/reconciler/v1alpha1/revision/resources/constants.go b/pkg/reconciler/v1alpha1/revision/resources/constants.go index 31b9e7dd1bb6..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" - defaultUserPort = 8080 userPortEnvName = "PORT" autoscalerPort = 8080 diff --git a/pkg/reconciler/v1alpha1/revision/resources/deploy.go b/pkg/reconciler/v1alpha1/revision/resources/deploy.go index bea3efc053cb..68c9e0c42aa8 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy.go @@ -129,20 +129,20 @@ func makePodSpec(rev *v1alpha1.Revision, loggingConfig *logging.Config, observab func createAndSetDefaultUserPort(userContainer *corev1.Container) { defaultUserPort := &corev1.ContainerPort{ - Name: userPortName, - ContainerPort: int32(defaultUserPort), + Name: v1alpha1.RevisionContainerUserPortName, + ContainerPort: int32(v1alpha1.RevisionContainerUserPortDefaultValue), } userContainer.Ports = append(userContainer.Ports, *defaultUserPort) } func getUserPort(rev *v1alpha1.Revision) (int32, bool) { for _, port := range rev.Spec.Container.Ports { - if port.Name == userPortName { + if port.Name == v1alpha1.RevisionContainerUserPortName { return port.ContainerPort, true } } - return defaultUserPort, false + return v1alpha1.RevisionContainerUserPortDefaultValue, false } func buildUserPortEnv(userPort int32) corev1.EnvVar { @@ -154,16 +154,6 @@ func buildUserPortEnv(userPort int32) corev1.EnvVar { return userPortEnv } -func buildQueueProxyExtraEnv(userPort *corev1.ContainerPort) []corev1.EnvVar { - queueContainerExtraEnv := []corev1.EnvVar{{ - Name: "USER_PORT", - Value: strconv.Itoa(int(userPort.ContainerPort)), - }, - } - - return queueContainerExtraEnv -} - 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 57e02d1aa7d8..ec534db42af4 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/deploy_test.go @@ -39,14 +39,14 @@ import ( var ( one int32 = 1 userPorts = []corev1.ContainerPort{{ - Name: userPortName, - ContainerPort: int32(defaultUserPort), + Name: v1alpha1.RevisionContainerUserPortName, + ContainerPort: int32(v1alpha1.RevisionContainerUserPortDefaultValue), }} // Expose containerPort as env PORT. userEnv = corev1.EnvVar{ Name: userPortEnvName, - Value: strconv.Itoa(defaultUserPort), + Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue), } ) @@ -75,7 +75,7 @@ func TestMakePodSpec(t *testing.T) { Image: "busybox", Ports: []corev1.ContainerPort{ { - Name: userPortName, + Name: v1alpha1.RevisionContainerUserPortName, ContainerPort: 8888, }, }, @@ -93,7 +93,7 @@ func TestMakePodSpec(t *testing.T) { Resources: userResources, Ports: []corev1.ContainerPort{ { - Name: userPortName, + Name: v1alpha1.RevisionContainerUserPortName, ContainerPort: 8888, }, }, @@ -421,7 +421,7 @@ func TestMakePodSpec(t *testing.T) { ReadinessProbe: &corev1.Probe{ Handler: corev1.Handler{ HTTPGet: &corev1.HTTPGetAction{ - Port: intstr.FromInt(defaultUserPort), + Port: intstr.FromInt(v1alpha1.RevisionContainerUserPortDefaultValue), Path: "/", }, }, @@ -723,7 +723,7 @@ func TestMakePodSpec(t *testing.T) { LivenessProbe: &corev1.Probe{ Handler: corev1.Handler{ TCPSocket: &corev1.TCPSocketAction{ - Port: intstr.FromInt(defaultUserPort), + Port: intstr.FromInt(v1alpha1.RevisionContainerUserPortDefaultValue), }, }, }, diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go index 6eee444157a1..2b49871c7bd7 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue_test.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue_test.go @@ -61,7 +61,7 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{}, userport: &corev1.ContainerPort{ Name: userPortEnvName, - ContainerPort: defaultUserPort, + ContainerPort: v1alpha1.RevisionContainerUserPortDefaultValue, }, want: &corev1.Container{ // These are effectively constant @@ -100,7 +100,7 @@ func TestMakeQueueContainer(t *testing.T) { // No logging level }, { Name: "USER_PORT", - Value: strconv.Itoa(defaultUserPort), + Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue), }}, }, }, { @@ -122,7 +122,7 @@ func TestMakeQueueContainer(t *testing.T) { }, userport: &corev1.ContainerPort{ Name: userPortEnvName, - ContainerPort: defaultUserPort, + ContainerPort: v1alpha1.RevisionContainerUserPortDefaultValue, }, want: &corev1.Container{ // These are effectively constant @@ -162,7 +162,7 @@ func TestMakeQueueContainer(t *testing.T) { // No logging level }, { Name: "USER_PORT", - Value: strconv.Itoa(defaultUserPort), + Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue), }}, }, }, { @@ -189,7 +189,7 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{}, userport: &corev1.ContainerPort{ Name: userPortEnvName, - ContainerPort: defaultUserPort, + ContainerPort: v1alpha1.RevisionContainerUserPortDefaultValue, }, want: &corev1.Container{ // These are effectively constant @@ -228,7 +228,7 @@ func TestMakeQueueContainer(t *testing.T) { // No logging level }, { Name: "USER_PORT", - Value: strconv.Itoa(defaultUserPort), + Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue), }}, }, }, { @@ -253,7 +253,7 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{}, userport: &corev1.ContainerPort{ Name: userPortEnvName, - ContainerPort: defaultUserPort, + ContainerPort: v1alpha1.RevisionContainerUserPortDefaultValue, }, want: &corev1.Container{ // These are effectively constant @@ -292,7 +292,7 @@ func TestMakeQueueContainer(t *testing.T) { Value: "error", // from logging config }, { Name: "USER_PORT", - Value: strconv.Itoa(defaultUserPort), + Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue), }}, }, }, { @@ -312,7 +312,7 @@ func TestMakeQueueContainer(t *testing.T) { cc: &config.Controller{}, userport: &corev1.ContainerPort{ Name: userPortEnvName, - ContainerPort: defaultUserPort, + ContainerPort: v1alpha1.RevisionContainerUserPortDefaultValue, }, want: &corev1.Container{ // These are effectively constant @@ -351,7 +351,7 @@ func TestMakeQueueContainer(t *testing.T) { // No logging level }, { Name: "USER_PORT", - Value: strconv.Itoa(defaultUserPort), + Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue), }}, }, }} diff --git a/test/e2e/autoscale_test.go b/test/e2e/autoscale_test.go index 55e3966c5a1b..50ef757c4afa 100644 --- a/test/e2e/autoscale_test.go +++ b/test/e2e/autoscale_test.go @@ -133,7 +133,7 @@ type testContext struct { domain string } -func setup(t *testing.T, userport int32, imageName string) *testContext { +func setup(t *testing.T) *testContext { //add test case specific name to its own logger logger := logging.GetContextLogger(t.Name()) clients := Setup(t) @@ -147,12 +147,11 @@ func setup(t *testing.T, userport int32, imageName string) *testContext { t.Fatalf("Unable to parse scale-to-zero-threshold as duration: %v", err) } - imagePath := test.ImagePath(imageName) + imagePath := test.ImagePath("autoscale") logger.Infof("Creating a new Route and Configuration") names, err := CreateRouteAndConfig(clients, logger, imagePath, &test.Options{ ContainerConcurrency: 10, - ContainerPorts: generateUserPort(logger, userport), }) if err != nil { t.Fatalf("Failed to create Route and Configuration: %v", err) @@ -206,18 +205,6 @@ func setup(t *testing.T, userport int32, imageName string) *testContext { } } -func generateUserPort(logger *logging.BaseLogger, port int32) []v1.ContainerPort { - containerPorts := []v1.ContainerPort{} - userPort := v1.ContainerPort{ - Name: "user-port", - ContainerPort: int32(port), - } - logger.Infof("set user port: %v", port) - - containerPorts = append(containerPorts, userPort) - return containerPorts -} - func assertScaleUp(ctx *testContext) { ctx.logger.Infof("The autoscaler spins up additional replicas when traffic increases.") err := generateTraffic(ctx, 20, 20*time.Second) @@ -272,18 +259,7 @@ func assertScaleDown(ctx *testContext) { } func TestAutoscaleUpDownUp(t *testing.T) { - ctx := setup(t, 8080, "autoscale") - stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger) - defer close(stopChan) - defer tearDown(ctx) - - assertScaleUp(ctx) - assertScaleDown(ctx) - assertScaleUp(ctx) -} - -func TestAutoscaleUpDownUpUserPort(t *testing.T) { - ctx := setup(t, 8888, "autoscale-userport") + ctx := setup(t) stopChan := DiagnoseMeEvery(15*time.Second, ctx.clients, ctx.logger) defer close(stopChan) defer tearDown(ctx) @@ -291,4 +267,4 @@ func TestAutoscaleUpDownUpUserPort(t *testing.T) { assertScaleUp(ctx) assertScaleDown(ctx) assertScaleUp(ctx) -} +} \ No newline at end of file diff --git a/test/e2e/helloworld_test.go b/test/e2e/helloworld_test.go index 74a8c02a1e5d..e7c016394a20 100644 --- a/test/e2e/helloworld_test.go +++ b/test/e2e/helloworld_test.go @@ -20,12 +20,14 @@ package e2e import ( "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" + "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -104,3 +106,119 @@ func TestHelloWorld(t *testing.T) { t.Fatalf("Failed to get Service name from Revision label") } } + +func TestHelloWorldUserPort(t *testing.T) { + var userPort int32 + userPort = 8888 + clients := Setup(t) + + //add test case specific name to its own logger + logger := logging.GetContextLogger("TestHelloWorld-UserPort") + + var imagePath = test.ImagePath("helloworld-userport") + + logger.Infof("Creating a new Route and Configuration") + names, err := CreateRouteAndConfig(clients, logger, imagePath, &test.Options{ + ContainerPorts: generateUserPort(logger, userPort), + }) + if err != nil { + t.Fatalf("Failed to create Route and Configuration: %v", err) + } + test.CleanupOnInterrupt(func() { TearDown(clients, names, logger) }, logger) + defer TearDown(clients, names, logger) + + logger.Infof("When the Revision can have traffic routed to it, the Route is marked as Ready.") + if err := test.WaitForRouteState(clients.ServingClient, names.Route, test.IsRouteReady, "RouteIsReady"); err != nil { + t.Fatalf("The Route %s was not marked as Ready to serve traffic: %v", names.Route, err) + } + + route, err := clients.ServingClient.Routes.Get(names.Route, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Error fetching Route %s: %v", names.Route, err) + } + domain := route.Status.Domain + + _, err = pkgTest.WaitForEndpointState( + clients.KubeClient, + logger, + domain, + pkgTest.Retrying(pkgTest.MatchesBody(helloWorldExpectedOutput), http.StatusNotFound), + "HelloWorldServesText", + test.ServingFlags.ResolvableDomain) + if err != nil { + t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, domain, helloWorldExpectedOutput, err) + } + + logger.Infof("Check user deployment's Ports and Env info.") + config, err := clients.ServingClient.Configs.Get(names.Config, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Configuration %s was not updated with the new revision: %v", names.Config, err) + } + deploymentName := + config.Status.LatestCreatedRevisionName + "-deployment" + deploy, err := clients.KubeClient.Kube.ExtensionsV1beta1().Deployments("serving-tests").Get(deploymentName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to get deployment %v, %v", deploymentName, err) + } + + userContainer, found := findUserContainer(deploy.Spec.Template.Spec.Containers, "user-container") + if !found { + t.Fatalf("Failed to find deployment %v's user container", deploymentName) + } + found = findExpectPortInContainer(userContainer, v1alpha1.RevisionContainerUserPortName, userPort) + if !found { + t.Fatalf("Failed to find deployment %v's user container's user-port", deploymentName) + } + found = findExpectEnvInContainer(userContainer, "PORT", strconv.Itoa(int(userPort))) + if !found { + t.Fatalf("Failed to find deployment %v's user container's PORT env", deploymentName) + } + + queueProxyContainer, found := findUserContainer(deploy.Spec.Template.Spec.Containers, "queue-proxy") + if !found { + t.Fatalf("Failed to find deployment %v's queue-proxy", deploymentName) + } + found = findExpectEnvInContainer(queueProxyContainer, "USER_PORT", strconv.Itoa(int(userPort))) + if !found { + t.Fatalf("Failed to find deployment %v's queue-proxy's USER_PORT env", deploymentName) + } +} + +func findUserContainer(containers []v1.Container, containerName string) (*v1.Container, bool) { + for _, container := range containers { + if container.Name == containerName { + return &container, true + } + } + return nil, false +} + +func findExpectPortInContainer(contaienr *v1.Container, expectPortName string, expectPortValue int32) bool { + for _, port := range contaienr.Ports { + if port.Name == expectPortName && port.ContainerPort == expectPortValue { + return true + } + } + return false +} + +func findExpectEnvInContainer(contaienr *v1.Container, expectEnvName string, expectEnvValue string) bool { + for _, env := range contaienr.Env { + if env.Name == expectEnvName && env.Value == expectEnvValue { + return true + } + } + return false +} + +func generateUserPort(logger *logging.BaseLogger, port int32) []v1.ContainerPort { + containerPorts := []v1.ContainerPort{} + userPort := v1.ContainerPort{ + Name: "user-port", + ContainerPort: int32(port), + } + logger.Infof("set user port: %v", port) + + containerPorts = append(containerPorts, userPort) + return containerPorts +} diff --git a/test/test_images/autoscale-userport/README.md b/test/test_images/autoscale-userport/README.md deleted file mode 100644 index a573906fceb0..000000000000 --- a/test/test_images/autoscale-userport/README.md +++ /dev/null @@ -1,12 +0,0 @@ -# Autoscale test image - -This directory contains the test image used in the autoscale e2e tests. - -The image contains a simple Go webserver, `autoscale.go`, that will, by default, listen on port `8888` and expose a service at `/`. - -When called, the server calculates the highest prime less than 40000000 and emits a message. - -## 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/autoscale-userport/autoscale.go b/test/test_images/autoscale-userport/autoscale.go deleted file mode 100644 index 990f4429f808..000000000000 --- a/test/test_images/autoscale-userport/autoscale.go +++ /dev/null @@ -1,104 +0,0 @@ -/* -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 - - http://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 ( - "fmt" - "log" - "math" - "net/http" - "sync" - "time" -) - -// Algorithm from https://stackoverflow.com/a/21854246 - -// Only primes less than or equal to N will be generated -func primes(N int) []int { - - var x, y, n int - nsqrt := math.Sqrt(float64(N)) - - isPrime := make([]bool, N) - - for x = 1; float64(x) <= nsqrt; x++ { - for y = 1; float64(y) <= nsqrt; y++ { - n = 4*(x*x) + y*y - if n <= N && (n%12 == 1 || n%12 == 5) { - isPrime[n] = !isPrime[n] - } - n = 3*(x*x) + y*y - if n <= N && n%12 == 7 { - isPrime[n] = !isPrime[n] - } - n = 3*(x*x) - y*y - if x > y && n <= N && n%12 == 11 { - isPrime[n] = !isPrime[n] - } - } - } - - for n = 5; float64(n) <= nsqrt; n++ { - if isPrime[n] { - for y = n * n; y < N; y += n * n { - isPrime[y] = false - } - } - } - - isPrime[2] = true - isPrime[3] = true - - primes := make([]int, 0, 1270606) - for x = 0; x < len(isPrime)-1; x++ { - if isPrime[x] { - primes = append(primes, x) - } - } - - // primes is now a slice that contains all primes numbers up to N - return primes -} - -func handler(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusOK) - var wg sync.WaitGroup - wg.Add(2) - go func() { - defer wg.Done() - p := primes(400000) - largest := p[len(p)-1] - msg := fmt.Sprintf("The largest prime under 400000 is %d. Enjoy your noodles!", largest) - fmt.Fprintf(w, msg) - log.Printf(msg) - }() - go func() { - defer wg.Done() - start := time.Now() - time.Sleep(time.Second) - msg := fmt.Sprintf("Slept for %v.", time.Since(start)) - fmt.Fprintf(w, msg) - log.Printf(msg) - }() - wg.Wait() -} - -func main() { - http.HandleFunc("/", handler) - http.ListenAndServe(":8888", nil) -} diff --git a/test/test_images/helloworld-userport/README.md b/test/test_images/helloworld-userport/README.md new file mode 100644 index 000000000000..2021a8d02b6b --- /dev/null +++ b/test/test_images/helloworld-userport/README.md @@ -0,0 +1,15 @@ +# Helloworld test image + +This directory contains the test image used in the helloworld e2e test. + +The image contains a simple Go webserver, `helloworld.go`, that will, by default, listen on port `8888` and expose a service at `/`. + +It used to test user-defined port. + +When called, the server emits a "hello world" message. + +## 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/helloworld-userport/helloworld.go b/test/test_images/helloworld-userport/helloworld.go new file mode 100644 index 000000000000..d12f63213a66 --- /dev/null +++ b/test/test_images/helloworld-userport/helloworld.go @@ -0,0 +1,37 @@ +/* +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" +) + +func handler(w http.ResponseWriter, r *http.Request) { + log.Print("Hello world received a request.") + fmt.Fprintln(w, "Hello World! How about some tasty noodles?") +} + +func main() { + flag.Parse() + log.Print("Hello world app started.") + + http.HandleFunc("/", handler) + http.ListenAndServe(":8888", nil) +} diff --git a/test/test_images/helloworld-userport/helloworld.yaml b/test/test_images/helloworld-userport/helloworld.yaml new file mode 100644 index 000000000000..47b01087c379 --- /dev/null +++ b/test/test_images/helloworld-userport/helloworld.yaml @@ -0,0 +1,44 @@ +# 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/helloworld-userport + 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 From dbe33ae8910cc0ca0b6fe68edc3101fa3548d8ec Mon Sep 17 00:00:00 2001 From: leonlm Date: Fri, 2 Nov 2018 14:04:36 +0800 Subject: [PATCH 7/8] change validation info && e2e test --- .../serving/v1alpha1/revision_validation.go | 10 +-- .../v1alpha1/revision_validation_test.go | 12 ++-- test/e2e/e2e.go | 66 +++++++++++++++++++ test/e2e/helloworld_shell_test.go | 52 ++++----------- test/e2e/helloworld_test.go | 27 ++++---- .../helloworld-userport/helloworld.yaml | 4 ++ 6 files changed, 107 insertions(+), 64 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go index 52fb17fc974a..4d969e5e400c 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation.go +++ b/pkg/apis/serving/v1alpha1/revision_validation.go @@ -140,17 +140,17 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError { // if user didn't set any port, it will set default port user-port=8080. if len(ports) > 1 { return &apis.FieldError{ - Message: "container ports set more than one", + Message: "More than one container ports is set", Paths: []string{"ports"}, - Details: "only can be set named \"user-port\" port", + Details: "Only a single port named \"user-port\" is allowed", } } - if len(ports) == 1 && ports[0].Name != "user-port"{ + if len(ports) == 1 && ports[0].Name != "user-port" { return &apis.FieldError{ - Message: fmt.Sprintf("unsupport port name %v", ports[0].Name), + Message: fmt.Sprintf("Port name %v is not allowed", ports[0].Name), Paths: []string{"ports"}, - Details: "only can be set named \"user-port\" port", + Details: "Only a single port named \"user-port\" is allowed", } } return nil diff --git a/pkg/apis/serving/v1alpha1/revision_validation_test.go b/pkg/apis/serving/v1alpha1/revision_validation_test.go index 899718023933..0519fd833b38 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation_test.go +++ b/pkg/apis/serving/v1alpha1/revision_validation_test.go @@ -75,9 +75,9 @@ func TestContainerValidation(t *testing.T) { }}, }, want: &apis.FieldError{ - Message: "container ports set more than one", + Message: "More than one container ports is set", Paths: []string{"ports"}, - Details: "only can be set named \"user-port\" port", + Details: "Only a single port named \"user-port\" is allowed", }, }, { name: "set wrong name port", @@ -88,9 +88,9 @@ func TestContainerValidation(t *testing.T) { }}, }, want: &apis.FieldError{ - Message: fmt.Sprintf("unsupport port name %v", "http"), + Message: fmt.Sprintf("Port name %v is not allowed", "http"), Paths: []string{"ports"}, - Details: "only can be set named \"user-port\" port", + Details: "Only a single port named \"user-port\" is allowed", }, }, { name: "has volumeMounts", @@ -172,9 +172,9 @@ func TestContainerValidation(t *testing.T) { Lifecycle: &corev1.Lifecycle{}, }, want: apis.ErrDisallowedFields("name", "resources", "volumeMounts", "lifecycle").Also(&apis.FieldError{ - Message: fmt.Sprintf("unsupport port name %v", "http"), + Message: fmt.Sprintf("Port name %v is not allowed", "http"), Paths: []string{"ports"}, - Details: "only can be set named \"user-port\" port", + Details: "Only a single port named \"user-port\" is allowed", }), }} diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go index a018583bd9a1..6009aed9781f 100644 --- a/test/e2e/e2e.go +++ b/test/e2e/e2e.go @@ -1,6 +1,10 @@ package e2e import ( + "io/ioutil" + "os" + "os/exec" + "strings" "testing" // Mysteriously required to support GCP auth (required by k8s libs). @@ -50,3 +54,65 @@ func CreateRouteAndConfig(clients *test.Clients, logger *logging.BaseLogger, ima err := test.CreateRoute(logger, clients, names) return names, err } + +// TearDownByYaml will delete resouce defined in yaml. +func TearDownByYaml(yamlFilename string, logger *logging.BaseLogger) { + if yamlFilename != "" { + exec.Command("kubectl", "delete", "-f", yamlFilename).Run() + os.Remove(yamlFilename) + } +} + +// CreateConfigAndRouteFromYaml will create Route and Config by yaml file. +func CreateConfigAndRouteFromYaml( + logger *logging.BaseLogger, + imageName, appYaml string, + configNamePlaceholder, routeNamePlaceholder, yamlImagePlaceholder, namespacePlaceholder string, +) (string, test.ResourceNames, error) { + logger.Infof("Creating manifest") + imagePath := test.ImagePath(imageName) + + var names test.ResourceNames + names.Config = test.AppendRandomString(configName, logger) + names.Route = test.AppendRandomString(routeName, logger) + + // Create manifest file. + newYaml, err := ioutil.TempFile("", "TempYaml") + if err != nil { + logger.Errorf("Failed to create temporary manifest: %v", err) + return "", names, err + } + newYamlFilename := newYaml.Name() + + // Populate manifets file with the real path to the container + yamlBytes, err := ioutil.ReadFile(appYaml) + if err != nil { + logger.Errorf("Failed to read file %s: %v", appYaml, err) + return newYamlFilename, names, err + } + + content := strings.Replace(string(yamlBytes), yamlImagePlaceholder, imagePath, -1) + content = strings.Replace(string(content), configNamePlaceholder, names.Config, -1) + content = strings.Replace(string(content), routeNamePlaceholder, names.Route, -1) + content = strings.Replace(string(content), namespacePlaceholder, test.ServingNamespace, -1) + + if _, err = newYaml.WriteString(content); err != nil { + logger.Errorf("Failed to write new manifest: %v", err) + return newYamlFilename, names, err + } + if err = newYaml.Close(); err != nil { + logger.Errorf("Failed to close new manifest file: %v", err) + return newYamlFilename, names, err + } + + logger.Infof("Manifest file is '%s'", newYamlFilename) + logger.Info("Deploying using kubectl") + + // Deploy using kubectl + if output, err := exec.Command("kubectl", "apply", "-f", newYamlFilename).CombinedOutput(); err != nil { + logger.Errorf("Error running kubectl: %v", strings.TrimSpace(string(output))) + return newYamlFilename, names, err + } + + return newYamlFilename, names, nil +} diff --git a/test/e2e/helloworld_shell_test.go b/test/e2e/helloworld_shell_test.go index 340245926f0c..f58c3c68abfe 100644 --- a/test/e2e/helloworld_shell_test.go +++ b/test/e2e/helloworld_shell_test.go @@ -20,8 +20,6 @@ package e2e import ( "bytes" - "io/ioutil" - "os" "os/exec" "strings" "testing" @@ -48,50 +46,24 @@ func noStderrShell(name string, arg ...string) string { return string(out) } -func cleanup(yamlFilename string, logger *logging.BaseLogger) { - exec.Command("kubectl", "delete", "-f", yamlFilename).Run() - os.Remove(yamlFilename) -} - func TestHelloWorldFromShell(t *testing.T) { //add test case specific name to its own logger logger := logging.GetContextLogger("TestHelloWorldFromShell") - imagePath := test.ImagePath("helloworld") - - logger.Infof("Creating manifest") - - // Create manifest file. - newYaml, err := ioutil.TempFile("", "helloworld") - if err != nil { - t.Fatalf("Failed to create temporary manifest: %v", err) - } - newYamlFilename := newYaml.Name() - defer cleanup(newYamlFilename, logger) - test.CleanupOnInterrupt(func() { cleanup(newYamlFilename, logger) }, logger) - // Populate manifets file with the real path to the container - yamlBytes, err := ioutil.ReadFile(appYaml) + newYamlFilename, names, err := CreateConfigAndRouteFromYaml(logger, + "helloworld", + appYaml, + "configuration-example", + "route-example", + yamlImagePlaceholder, + namespacePlaceholder, + ) if err != nil { - t.Fatalf("Failed to read file %s: %v", appYaml, err) + t.Fatalf("Failed to create configuration and route: %v", err) } - content := strings.Replace(string(yamlBytes), yamlImagePlaceholder, imagePath, -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) - } - if err = newYaml.Close(); err != nil { - t.Fatalf("Failed to close new manifest file: %v", err) - } - - logger.Infof("Manifest file is '%s'", newYamlFilename) - logger.Info("Deploying using kubectl") - - // Deploy using kubectl - if output, err := exec.Command("kubectl", "apply", "-f", newYamlFilename).CombinedOutput(); err != nil { - t.Fatalf("Error running kubectl: %v", strings.TrimSpace(string(output))) - } + defer TearDownByYaml(newYamlFilename, logger) + test.CleanupOnInterrupt(func() { TearDownByYaml(newYamlFilename, logger) }, logger) logger.Info("Waiting for ingress to come up") @@ -100,7 +72,7 @@ func TestHelloWorldFromShell(t *testing.T) { serviceHost := "" timeout := ingressTimeout for (serviceIP == "" || serviceHost == "") && timeout >= 0 { - serviceHost = noStderrShell("kubectl", "get", "rt", "route-example", "-o", "jsonpath={.status.domain}", "-n", test.ServingNamespace) + serviceHost = noStderrShell("kubectl", "get", "rt", names.Route, "-o", "jsonpath={.status.domain}", "-n", test.ServingNamespace) serviceIP = noStderrShell("kubectl", "get", "svc", "knative-ingressgateway", "-n", "istio-system", "-o", "jsonpath={.status.loadBalancer.ingress[*]['ip']}") time.Sleep(checkInterval) diff --git a/test/e2e/helloworld_test.go b/test/e2e/helloworld_test.go index e7c016394a20..f68b0c410e2b 100644 --- a/test/e2e/helloworld_test.go +++ b/test/e2e/helloworld_test.go @@ -19,16 +19,15 @@ limitations under the License. package e2e import ( - "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" "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "net/http" + "strconv" + "testing" ) const ( @@ -115,17 +114,19 @@ func TestHelloWorldUserPort(t *testing.T) { //add test case specific name to its own logger logger := logging.GetContextLogger("TestHelloWorld-UserPort") - var imagePath = test.ImagePath("helloworld-userport") - - logger.Infof("Creating a new Route and Configuration") - names, err := CreateRouteAndConfig(clients, logger, imagePath, &test.Options{ - ContainerPorts: generateUserPort(logger, userPort), - }) + newYamlFilename, names, err := CreateConfigAndRouteFromYaml(logger, + "helloworld-userport", + "../test_images/helloworld-userport/helloworld.yaml", + "configuration-example", + "route-example", + "github.com/knative/serving/test_images/helloworld-userport", + namespacePlaceholder, + ) if err != nil { - t.Fatalf("Failed to create Route and Configuration: %v", err) + t.Fatalf("Failed to create configuration and route: %v", err) } - test.CleanupOnInterrupt(func() { TearDown(clients, names, logger) }, logger) - defer TearDown(clients, names, logger) + defer TearDownByYaml(newYamlFilename, logger) + test.CleanupOnInterrupt(func() { TearDownByYaml(newYamlFilename, logger) }, logger) logger.Infof("When the Revision can have traffic routed to it, the Route is marked as Ready.") if err := test.WaitForRouteState(clients.ServingClient, names.Route, test.IsRouteReady, "RouteIsReady"); err != nil { diff --git a/test/test_images/helloworld-userport/helloworld.yaml b/test/test_images/helloworld-userport/helloworld.yaml index 47b01087c379..23f5837d3026 100644 --- a/test/test_images/helloworld-userport/helloworld.yaml +++ b/test/test_images/helloworld-userport/helloworld.yaml @@ -27,6 +27,10 @@ spec: # This is the Go import path for the binary to containerize # and substitute here. image: github.com/knative/serving/test_images/helloworld-userport + ports: + # Test user-defined port + - name: user-port + containerPort: 8888 readinessProbe: httpGet: path: / From ad4bec8ec06bd839eb062e3c46de60b2023da591 Mon Sep 17 00:00:00 2001 From: leonlm Date: Mon, 5 Nov 2018 19:52:31 +0800 Subject: [PATCH 8/8] more review comments modify --- pkg/apis/serving/v1alpha1/revision_validation.go | 6 +++--- pkg/reconciler/v1alpha1/revision/resources/queue.go | 3 +-- test/e2e/autoscale_test.go | 2 +- test/e2e/helloworld_test.go | 12 ------------ 4 files changed, 5 insertions(+), 18 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/revision_validation.go b/pkg/apis/serving/v1alpha1/revision_validation.go index 4d969e5e400c..79d441cd1768 100644 --- a/pkg/apis/serving/v1alpha1/revision_validation.go +++ b/pkg/apis/serving/v1alpha1/revision_validation.go @@ -122,7 +122,7 @@ func validateContainer(container corev1.Container) *apis.FieldError { errs = errs.Also(apis.ErrDisallowedFields(ignoredFields...)) } if err := validateContainerPorts(container.Ports); err != nil { - errs = errs.Also(err) + errs = errs.Also(err.ViaField("ports")) } // Validate our probes if err := validateProbe(container.ReadinessProbe).ViaField("readinessProbe"); err != nil { @@ -141,7 +141,7 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError { if len(ports) > 1 { return &apis.FieldError{ Message: "More than one container ports is set", - Paths: []string{"ports"}, + Paths: []string{apis.CurrentField}, Details: "Only a single port named \"user-port\" is allowed", } } @@ -149,7 +149,7 @@ func validateContainerPorts(ports []corev1.ContainerPort) *apis.FieldError { if len(ports) == 1 && ports[0].Name != "user-port" { return &apis.FieldError{ Message: fmt.Sprintf("Port name %v is not allowed", ports[0].Name), - Paths: []string{"ports"}, + Paths: []string{apis.CurrentField}, Details: "Only a single port named \"user-port\" is allowed", } } diff --git a/pkg/reconciler/v1alpha1/revision/resources/queue.go b/pkg/reconciler/v1alpha1/revision/resources/queue.go index b8ba3086357f..abafc6b41156 100644 --- a/pkg/reconciler/v1alpha1/revision/resources/queue.go +++ b/pkg/reconciler/v1alpha1/revision/resources/queue.go @@ -85,7 +85,7 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a loggingLevel = ll.String() } - queueContainer := &corev1.Container{ + return &corev1.Container{ Name: queueContainerName, Image: controllerConfig.QueueSidecarImage, Resources: queueResources, @@ -128,5 +128,4 @@ func makeQueueContainer(rev *v1alpha1.Revision, loggingConfig *logging.Config, a Value: strconv.Itoa(int(userport)), }}, } - return queueContainer } diff --git a/test/e2e/autoscale_test.go b/test/e2e/autoscale_test.go index 50ef757c4afa..43c7f8c7961f 100644 --- a/test/e2e/autoscale_test.go +++ b/test/e2e/autoscale_test.go @@ -267,4 +267,4 @@ func TestAutoscaleUpDownUp(t *testing.T) { assertScaleUp(ctx) assertScaleDown(ctx) assertScaleUp(ctx) -} \ No newline at end of file +} diff --git a/test/e2e/helloworld_test.go b/test/e2e/helloworld_test.go index f68b0c410e2b..603848ff6e72 100644 --- a/test/e2e/helloworld_test.go +++ b/test/e2e/helloworld_test.go @@ -211,15 +211,3 @@ func findExpectEnvInContainer(contaienr *v1.Container, expectEnvName string, exp } return false } - -func generateUserPort(logger *logging.BaseLogger, port int32) []v1.ContainerPort { - containerPorts := []v1.ContainerPort{} - userPort := v1.ContainerPort{ - Name: "user-port", - ContainerPort: int32(port), - } - logger.Infof("set user port: %v", port) - - containerPorts = append(containerPorts, userPort) - return containerPorts -}