Allow user-defined ports on Configuration

[dgerd]

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
Fixes #1241

[googlebot]

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.

[knative-prow-robot]

@dgerd: 0 warnings.

In response to this:

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

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.

[dgerd]

/assign @tcnghia

Can you take a pass at this?

cc @evankanderson in case you wanted to take a look as you chimed in on the original PR.

[dgerd]

cc @tanzeeb as this is somewhat related to #2539. This PR puts down the prerequisite work for plumbing content-negotiation through the Port[0].Name field.

[tanzeeb]

Hey @dgerd , this is great! Thanks for looking into the named ports as well.

One question, you mentioned

remove "activator" istio-proxy , set "sidecar.istio.io/inject" to false

Is that still part of this PR?

[tcnghia]

@tanzeeb removing activator was part of the squashed commit messages from @Leon-Liangming's PR. I think that has been reverted in a following commit in the same PR.

[dgerd]

/test pull-knative-serving-upgrade-tests

[tcnghia]

/lgtm
/approve

[dgerd]

/test pull-knative-serving-upgrade-tests

[dgerd]

/test pull-knative-serving-integration-tests

[dgerd]

/cc @evankanderson

Do you mind taking a pass through this to make sure this correctly implements the spec? It is still missing the Docker EXPOSE lookup from #2258, but otherwise I believe it matches the behavior both defined in the issue and the runtime spec.

[evankanderson]

This LGTM; I have a few comments, but I don't think any are blocking.

/approve
/hold for comment review/response

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgerd, evankanderson, tcnghia

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:

• cmd/queue/OWNERS [evankanderson,tcnghia]
• pkg/apis/OWNERS [evankanderson]
• pkg/reconciler/OWNERS [evankanderson,tcnghia]
• test/OWNERS [evankanderson]

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

[evankanderson]

Also, the CLA seems angry -- do you know why? (Possibly due to accepted suggestions?)

[tcnghia]

@evankanderson my understanding is that the CLA doesn't like it when @dgerd pushed changes from a previous (inactive) PR that weren't his.

[dgerd]

@tcnghia is correct. It does not like that this is based on changes from #2190.

[knative-metrics-robot]

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.1%	94.4%	1.3

[mattmoor]

@evankanderson I'll bypass the CLA bot once things look good. Dan took this over from another PR where the author had signed the CLA, but then went AWOL.

[dgerd]

/retest

[dgerd]

/test pull-knative-serving-integration-tests

[adrcunha]

/retest

[evankanderson]

/lgtm

One followup comment, but this mostly looks good.

[mattmoor]

My understanding is that the outstanding comment is going to go into a follow up, so I'm going to bypass CI to get this merged, since Prow won't be able to bypass the CLA check.

[bradhoekstra]

@dgerd What happens if the user picks 8012 (RequestQueuePort) or 8022 (RequestQueueAdminPort) as their port?

[dgerd]

@bradhoekstra I will create a PR to disallow those two fields.

I also created a bug for the QueueProxy as choosing those ports actually sort-of worked or at least passed tests.

#2752

[gongzili456]

so, how to use user_port config, my knative version is 0.2.3, I try set it in Dockerfile and deployment file, not work.

[dgerd]

This feature has not been cherry picked into 0.2.3. It should go out in the 0.3 release in early January. When available you can set it in containerPorts within your RevisionTemplate.

…

On Fri, Dec 28, 2018 at 8:39 PM liuguili ***@***.***> wrote: so, how to use user_port config, my knative version is 0.2.3, I try set it in Dockerfile and deployment file, not work. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2642 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ApafMkegLuf2b3ATlod9SSxNu8pHZTvXks5u9tXFgaJpZM4ZEod6> .