Support set user-defined port to queue-proxy #2190

Leon-Liangming · 2018-10-09T08:45:42Z

Proposed Changes

Support set user-defined port to queue-proxy

Release Note

googlebot · 2018-10-09T08:45:48Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

knative-prow-robot

@Leon-Liangming: 0 warnings.

In response to this:

Fixes #2101

Proposed Changes

Support set user-defined port to queue-proxy

Release Note

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Leon-Liangming · 2018-10-09T08:55:36Z

I signed it!

googlebot · 2018-10-09T08:55:38Z

CLAs look good, thanks!

Leon-Liangming · 2018-10-09T09:13:08Z

/assign @josephburnett @mattmoor

Leon-Liangming · 2018-10-11T01:27:25Z

/assign @ZhiminXiang

mattmoor · 2018-10-11T03:18:35Z

/ok-to-test

ZhiminXiang

We should be careful about releasing this PR and document it clearly in our release notes since this PR directly affects current user behavior on the configuration of user-container port.

cmd/queue/main.go

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

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

Leon-Liangming · 2018-10-14T02:26:43Z

/test pull-knative-serving-unit-tests

Leon-Liangming · 2018-10-14T02:26:55Z

/test pull-knative-serving-integration-tests

googlebot · 2018-10-15T02:36:10Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

googlebot · 2018-10-15T02:54:28Z

CLAs look good, thanks!

steuhs · 2018-10-15T19:36:44Z

/test pull-knative-serving-go-coverage-dev

ZhiminXiang · 2018-10-15T23:20:46Z

/lgtm
Assign to @tcnghia to take another look and approve.

tcnghia · 2018-10-16T16:28:35Z

/approve

/assign @josephburnett

Leon-Liangming · 2018-10-17T06:54:43Z

/assign @mattmoor

mattmoor

First, thanks! Awesome to see this.

couple nits, which I think will simplify this a bit (both conceptually and the testing diffs).

I'd also like to see us add e2e coverage for this, as it is something that could break without tripping unit tests. Suggestions:

Add a simple conformance test (copy the "Service" test and specify a port via env var).
Modify the autoscaling test to do the same with a randomly chosen port (my interest in this test is it's exercising of the activator).

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

knative-metrics-robot · 2018-11-01T07:34:44Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_validation.go	93.8%	94.2%	0.4
test/test_images/helloworld-userport/helloworld.go	Do not exist	0.0%

tcnghia

This is looking really close. Thanks a lot!

I only have some more minor comments regarding the warning messages.

pkg/apis/serving/v1alpha1/revision_validation.go

test/e2e/helloworld_test.go

tcnghia · 2018-11-02T05:54:27Z

/approve

knative-prow-robot · 2018-11-02T05:54:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Leon-Liangming, tcnghia
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tcnghia · 2018-11-02T05:55:13Z

/assign @adrcunha
for test/OWNERS

/assign @mattmoor
for apis/OWNERS

knative-metrics-robot · 2018-11-02T06:10:26Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_validation.go	93.8%	94.2%	0.4
test/test_images/helloworld-userport/helloworld.go	Do not exist	0.0%

mattmoor

/assign @dgerd

Dan, this touches on two spaces you are wading into. Can you TAL?

pkg/apis/serving/v1alpha1/revision_validation.go

mattmoor · 2018-11-02T14:55:41Z

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

+	// Expose containerPort as env PORT.
+	userEnv = corev1.EnvVar{
+		Name:  userPortEnvName,
+		Value: strconv.Itoa(v1alpha1.RevisionContainerUserPortDefaultValue),


It feels like the most flexible/stable way to set this env var would be through the downward API, if possible.

this "PORT" env create by func buildUserPortEnv in deploy.go, maybe use this func is ok, but If we want to use this func, we need to change this internal func to global func .
I think if more tests need this env in the future, we can write a test func for this.

Any further suggestions here?

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

mattmoor · 2018-11-02T15:04:13Z

test/test_images/helloworld-userport/helloworld.yaml

+# limitations under the License.
+
+apiVersion: serving.knative.dev/v1alpha1
+kind: Configuration


Can this use Service instead?

I think Configuration is enough to my e2e test, copy from "helloworld_shell_test.go". I just want to do the same test about "helloworld_shell_test.go" or "TestHelloWorld" use user-defined port.

test/e2e/autoscale_test.go

mattmoor · 2018-11-02T15:12:54Z

test/e2e/e2e.go

+}
+
+// CreateConfigAndRouteFromYaml will create Route and Config by yaml file.
+func CreateConfigAndRouteFromYaml(


Can we create/delete using the same methods as other tests?

If we are going to change how we deploy test stuff, I'd rather we do it holistically (and not in this already large change). cc @dgerd

I think it had cover the all tests, Only TestHelloWorldFromShell and TestHelloWorldUserPort create resource by yaml now.

test/e2e/helloworld_test.go

knative-metrics-robot · 2018-11-05T11:55:32Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_validation.go	93.8%	94.2%	0.4
test/test_images/helloworld-userport/helloworld.go	Do not exist	0.0%

knative-metrics-robot · 2018-11-05T12:03:06Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/serving/v1alpha1/revision_validation.go	93.8%	94.2%	0.4
test/test_images/helloworld-userport/helloworld.go	Do not exist	0.0%

dgerd

Thanks for all the work here. This is a great improvement!

dgerd · 2018-11-06T21:37:51Z

pkg/apis/serving/v1alpha1/revision_validation.go

+		}
+	}
+
+	if len(ports) == 1 && ports[0].Name != "user-port" {


We should actually enforce that there is no name on the port for now. We can preserve the name 'user-port' on the deployment; however, the name is intended to be used for HTTP negotiation when specified on Configuration. Allowing it or requiring it to be 'user-port' would not allow us to implement the runtime contract. See https://github.com/knative/serving/blob/master/docs/runtime-contract.md#inbound-network-connectivity

so it means accept any name of the port ?
hmm, it's about how to set user-port by user, it seems we didn't discuss about that clearly. @mattmoor @evankanderson Do you have any suggestions here?

To clarify my understanding of the contract, there should only ever be 1 port in the Configuration Spec (or configuration section of Service Spec). Configuration.Spec.Container.Ports[0].Name should either be "" or "http1" or "h2c". Since we currently only implement automatic detection it probably makes sense to only accept "" for Name.

When the revision is created by Knative, the revision creates a K8s Deployment object from the spec. The port name on the Deployment is currently being set to 'user-port', and I think it makes sense to keep this set for clarity that the 'user-port' containerPort on the deployment was set from Configuration.Spec.Container.Ports[0].

To quote the spec (because apparently I had deep discussions about this 6 months ago, and was smarter than I am when looking at this issue for 10 minutes today):

Only one inbound containerPort SHALL be specified in the core.v1.Container specification. The hostPort parameter SHOULD NOT be set by the developer or the platform provider, as it can interfere with ingress autoscaling. Regardless of its source, the selected port will be made available in the PORT environment variable.

The platform provider SHOULD configure the platform to perform HTTPS termination and protocol transformation e.g. between QUIC or HTTP/2 and HTTP/1.1. Developers should not need to implement multiple transports between the platform and their code. Unless overridden by setting the name field on the inbound port, the platform will perform automatic detection as described above. If the core.v1.Container.ports[0].name is set to one of the following values, HTTP negotiation will be disabled and the following protocol will be used:

http1: HTTP/1.1 transport and will not attempt to upgrade to h2c..

h2c: HTTP/2 transport, as described in section 3.4 of the HTTP2 spec (Starting HTTP/2 with Prior Knowledge)

Developers SHOULD prefer to use automatic content negotiation where available, and MUST NOT set the name field to arbitrary values, as additional transports may be defined in the future.

All of the above is about the Container object specified in the Revision. The underlying implementation (e.g. creating a Deployment to back the Revision) can use whatever names it wants. The content-negotiation information needs to be forwarded to queue-proxy (this is not done today).

dgerd · 2018-11-06T21:52:56Z

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

+
+	userPort, found := getUserPort(rev)
+	if !found {
+		createAndSetDefaultUserPort(userContainer)


The logic here seems a bit strange that you return the default port when not found, but then don't use in createAndSetDefaultUserPort.

To me it would feel a bit more natural if getUserPort returned 0 for userPort when false, and createAndSetDefaultUserPort returned the default userPort. Otherwise you are relying on userPort to be equal to what createAndSetDefaultUserPort set which could easily become a bug.

I get your point.
According to @mattmoor's comment,
#discussion_r227053050

Instead of passing in additional EnvVars can we instead factor a function getUserPort(Revision) -> int64 and call it both here and queue.go?

get port from revision, used in two places, userPort need to get default port if can't find userport.
I change it to use getUserPort's result in createAndSetDefaultUserPort to resolve this pb.

dgerd · 2018-11-06T22:42:38Z

test/test_images/helloworld-userport/helloworld.yaml

+        readinessProbe:
+          httpGet:
+            path: /
+          initialDelaySeconds: 3


It is recommended that this be set to 0. Since this is an example that I imagine may be copied from in the future it would be nice to have this set to 0 if possible. See https://github.com/knative/serving/blob/master/docs/runtime-contract.md#meta-requests

dgerd · 2018-11-06T22:49:33Z

test/conformance/service_test.go

+	container.Env = append(container.Env, portEnv)
+}
+
+func assertServiceUserPortSet(t *testing.T, clients *test.Clients, names test.ResourceNames, assertUserPort string) {


The types of assertions here would be more appropriate for e2e tests. We can keep them here with a flag; however, the conformance tests must not depend on K8s resources. They should only make sure that KNative objects have the correct state (i.e. Configuration, Revision, Route, Service) See https://github.com/knative/serving/blob/master/test/conformance/README.md

I get it , I will remove it, and this assert had coverd by e2e test.

dgerd · 2018-11-06T22:50:30Z

test/conformance/service_test.go

+	}
+
+	if userContainerPortEnv.Value == assertUserPort {
+		t.Fatalf("user container's port is not expect. %v", userContainerPortEnv.Value)


It is helpful to have both the expected value and observed value. Can you add the assertUserPort value here and below?

dgerd · 2018-11-06T22:54:21Z

test/conformance/service_test.go

+	}
+	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.")


Rather than checking the set port variable on the kubernetes object you could have a test container that starts up on $PORT and returns "$PORT" when called. You could validate that when the service is ready the container returns success (200) and the port reported back is the port that was specified.

dgerd · 2018-11-06T23:00:41Z

test/e2e/helloworld_test.go

+		t.Fatalf("Configuration %s was not updated with the new revision: %v", names.Config, err)
+	}
+	deploymentName :=
+		config.Status.LatestCreatedRevisionName + "-deployment"


nit: can you pull this out to a getDeployment method in test?

dgerd · 2018-11-06T23:01:25Z

test/test_images/helloworld-userport/helloworld.go

+	log.Print("Hello world app started.")
+
+	http.HandleFunc("/", handler)
+	http.ListenAndServe(":8888", nil)


Can you listen on the environment variable port rather than 8888? That would be further validation that the environment variable is completely plumbed through.

dgerd · 2018-11-14T20:15:28Z

Checking in to see how this is going. Is there anything I can do to help here?

This is a big part of #2258 , and would love to see this get checked in.

mattmoor · 2018-11-27T18:28:38Z

@Leon-Liangming Any updates here?

dgerd · 2018-11-29T20:58:29Z

@Leon-Liangming

I have some time and a desire to get this merged into master for the 0.3 release. I am going to pull your commits, rebase them, and make the suggested changes above. I will reference this pull request with the new one.

Let me know if you have any questions or concerns.

This change allows for a user to add a port to their Container object in Configuration. The single port provided is used by the queue-proxy to as the port used to connect to the user container. This port is available as environment variable "PORT" on the user container and enviornment variable "USER_PORT" on the queue-proxy. This change is built upon PR knative#2190 by Leon-Liangming. It squashes the following commits: * change queue-proxy user-port env name * rebase master, fix uint test * remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false * add e2e test for user-port && review comments modify && open acitvator's istio-proxy * set more validation info about user-port && modify e2e test * change validation info && e2e test * more review comments modify * Allow only 1 unnamed port * Passing conformance tests Related to knative#2258

This change allows for a user to add a port to their Container object in Configuration. The single port provided is used by the queue-proxy to as the port used to connect to the user container. This port is available as environment variable "PORT" on the user container and environment variable "USER_PORT" on the queue-proxy. This change is built upon PR knative#2190 by Leon-Liangming. It squashes the following commits: * change queue-proxy user-port env name * rebase master, fix uint test * remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false * add e2e test for user-port && review comments modify && open acitvator's istio-proxy * set more validation info about user-port && modify e2e test * change validation info && e2e test * more review comments modify * Allow only 1 unnamed port * Passing conformance tests Related to knative#2258

This change allows for a user to add a port to their Container object in Configuration. The single port provided is used by the queue-proxy to as the port used to connect to the user container. This port is available as environment variable "PORT" on the user container and environment variable "USER_PORT" on the queue-proxy. This change is built upon PR knative#2190 by Leon-Liangming. It squashes the following commits: * change queue-proxy user-port env name * rebase master, fix uint test * remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false * add e2e test for user-port && review comments modify && open acitvator's istio-proxy * set more validation info about user-port && modify e2e test * change validation info && e2e test * more review comments modify * Allow only 1 unnamed port * Passing conformance tests Related to knative#2258 Reduce number of type conversions Allow h2c and http1 on for port name Code Review Comments Clean up port validation

mattmoor · 2018-12-15T23:05:40Z

Closing this in favor of Dan's PR, which picks up where this leaves off.

This change allows for a user to add a port to their Container object in Configuration. The single port provided is used by the queue-proxy to as the port used to connect to the user container. This port is available as environment variable "PORT" on the user container and environment variable "USER_PORT" on the queue-proxy. This change is built upon PR knative#2190 by Leon-Liangming. It squashes the following commits: * change queue-proxy user-port env name * rebase master, fix uint test * remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false * add e2e test for user-port && review comments modify && open acitvator's istio-proxy * set more validation info about user-port && modify e2e test * change validation info && e2e test * more review comments modify * Allow only 1 unnamed port * Passing conformance tests Related to knative#2258 Reduce number of type conversions Allow h2c and http1 on for port name Code Review Comments Clean up port validation

This change allows for a user to add a port to their Container object in Configuration. The single port provided is used by the queue-proxy to as the port used to connect to the user container. This port is available as environment variable "PORT" on the user container and environment variable "USER_PORT" on the queue-proxy. This change is built upon PR #2190 by Leon-Liangming. It squashes the following commits: * change queue-proxy user-port env name * rebase master, fix uint test * remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false * add e2e test for user-port && review comments modify && open acitvator's istio-proxy * set more validation info about user-port && modify e2e test * change validation info && e2e test * more review comments modify * Allow only 1 unnamed port * Passing conformance tests Related to #2258 Reduce number of type conversions Allow h2c and http1 on for port name Code Review Comments Clean up port validation

knative-prow-robot requested review from jonjohnsonjr and josephburnett October 9, 2018 08:45

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 9, 2018

knative-prow-robot reviewed Oct 9, 2018

View reviewed changes

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 9, 2018

knative-prow-robot assigned josephburnett and mattmoor Oct 9, 2018

mattmoor assigned ZhiminXiang Oct 9, 2018

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 11, 2018

ZhiminXiang reviewed Oct 11, 2018

View reviewed changes

cmd/queue/main.go Outdated Show resolved Hide resolved

pkg/reconciler/v1alpha1/revision/resources/deploy.go Outdated Show resolved Hide resolved

pkg/reconciler/v1alpha1/revision/resources/deploy.go Outdated Show resolved Hide resolved

ZhiminXiang reviewed Oct 11, 2018

View reviewed changes

pkg/reconciler/v1alpha1/revision/resources/queue.go Outdated Show resolved Hide resolved

Leon-Liangming force-pushed the user-port branch from 8a0461a to c76f7a7 Compare October 15, 2018 02:54

Leon-Liangming force-pushed the user-port branch from ab7eb1a to 341b6d3 Compare October 15, 2018 11:44

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2018

ZhiminXiang assigned tcnghia Oct 15, 2018

mattmoor reviewed Oct 22, 2018

View reviewed changes

pkg/reconciler/v1alpha1/revision/resources/deploy.go Outdated Show resolved Hide resolved

pkg/reconciler/v1alpha1/revision/resources/deploy.go Outdated Show resolved Hide resolved

tcnghia reviewed Nov 1, 2018

View reviewed changes

knative-prow-robot assigned adrcunha Nov 2, 2018

change validation info && e2e test

dbe33ae

mattmoor reviewed Nov 2, 2018

View reviewed changes

knative-prow-robot assigned dgerd Nov 2, 2018

more review comments modify

ad4bec8

Leon-Liangming force-pushed the user-port branch from de796c6 to ad4bec8 Compare November 5, 2018 12:00

dgerd suggested changes Nov 6, 2018

View reviewed changes

mattmoor added this to the Serving 0.3 milestone Nov 29, 2018

dgerd mentioned this pull request Dec 5, 2018

Allow user-defined ports on Configuration #2642

Merged

mattmoor closed this Dec 15, 2018

dgerd mentioned this pull request Feb 14, 2019

Add dgerd to list of serving-api-approvers #3228

Merged

Support set user-defined port to queue-proxy #2190

Support set user-defined port to queue-proxy #2190

Conversation

Leon-Liangming commented Oct 9, 2018

Proposed Changes

googlebot commented Oct 9, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Leon-Liangming commented Oct 9, 2018

googlebot commented Oct 9, 2018

Leon-Liangming commented Oct 9, 2018

Leon-Liangming commented Oct 11, 2018

mattmoor commented Oct 11, 2018

ZhiminXiang left a comment

Choose a reason for hiding this comment

Leon-Liangming commented Oct 14, 2018

Leon-Liangming commented Oct 14, 2018

googlebot commented Oct 15, 2018

googlebot commented Oct 15, 2018

steuhs commented Oct 15, 2018

ZhiminXiang commented Oct 15, 2018

tcnghia commented Oct 16, 2018

Leon-Liangming commented Oct 17, 2018

mattmoor left a comment

Choose a reason for hiding this comment

knative-metrics-robot commented Nov 1, 2018

tcnghia left a comment

Choose a reason for hiding this comment

tcnghia commented Nov 2, 2018

knative-prow-robot commented Nov 2, 2018

tcnghia commented Nov 2, 2018

knative-metrics-robot commented Nov 2, 2018

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leon-Liangming Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leon-Liangming Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knative-metrics-robot commented Nov 5, 2018

knative-metrics-robot commented Nov 5, 2018

dgerd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leon-Liangming Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Leon-Liangming Nov 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgerd commented Nov 14, 2018

mattmoor commented Nov 27, 2018

dgerd commented Nov 29, 2018

mattmoor commented Dec 15, 2018

Leon-Liangming Nov 5, 2018 •

edited

Loading

Leon-Liangming Nov 5, 2018 •

edited

Loading

Leon-Liangming Nov 8, 2018 •

edited

Loading

Leon-Liangming Nov 8, 2018 •

edited

Loading