Set $PORT for containers

[evankanderson]

(Copied from an earlier discussion about 2 months ago which didn't make it into an issue somehow.)

Our runtime spec says that the $PORT environment variable WILL indicate which port the environment will attempt to connect the container on. Ideally, all our customers would read our documentation and consume $PORT without needing any external assistance (flags, config, etc).

Unfortunately, our users are in as much of a rush as we are, so we should try to choose our defaults so that the maximum number of cases just work. Based on our experience, we default the containerPort to 8080 if unset, to catch the common use case. In some other cases, the container may expose a flag which allows the user to choose which port to run the server on. The user can set this flag in their configuration to $PORT to get the same behavior as our ideal, with the cost of one extra command-line flag.

In other cases, the built container may expose some other port which is hard-coded; if we are able to parse the metadata, we could extract the port from the EXPOSE command. If all of these fail, the customer should be able to set container.ports[0].containerPort to the hard-coded port in their docker image.

Expected Behavior

The PORT environment variable is set based on our best determination for the containerPort:

1. User's explicit yaml.
2. Docker EXPOSE metadata from image.
3. Default of 8080.

Actual Behavior

We always assume 8080.

See also #2101, which is a slightly different issue about our ability to consume the reasonable value for containerPort.

[mattmoor]

/assign @dgerd

[mattmoor]

Moving the remaining scope to 0.4. We've landed what we wanted in 0.3.

[markusthoemmes]

Should we only take the EXPOSEd port if there is only one port exposed? Or shall we take the first one in that case?

[dgerd]

After thinking about this a bit, I am personally not sure if trying to take EXPOSE is a good idea here. In the best case scenario it can simplify configuration, but in many cases it could make configuring port more difficult or cause unintentional behavior that would be difficult to debug. This behavior also doesn't seem to be required by our runtime contract.

If using a base image that has EXPOSE (or later adds EXPOSE), a new revision could unintentionally change your port causing your application to break on upgrade. I think this potential may force many to specify port rather than relying on the default. A static default value seems easier to manage and rely upon for large deployments of Knative.

We also have the challenge as you are pointing out that many may EXPOSE multiple ports in which case we would have to "guess" which would most likely amount to taking the first one or last one. I could see arguments for both of these options; however, either one we go with may be unintuitive to a user.

All that said I was not in the earlier discussion Evan mentions in the original post of this issue. Perhaps these concerns were already discussed.

@evankanderson @mattmoor Do you guys have any thoughts here?

[markusthoemmes]

+1 to @dgerd. The barrier of actually configuring the PORT seems so low compared to the amount of false positives and "surprise" we could get through inferring stuff through EXPOSE that I'm also inclined to take that out of scope.

[evankanderson]

If we're falling back to EXPOSE, I'd expect only a single port to be present (or for the Revision to be in an error state). I'm not sure I understand how changing the PORT (and expected user container port) would cause the application to break -- Kubernetes already performs a network translation when mapping from services to ContainerPorts, and the ingress to the whole system will continue to be 80/443 via the HTTP ingress. Such a break should also be visible in both the Revision status and Istio healthcheck such that users would avoid rolling out the broken Revision. Is there a specific breakage or scenario here that you're thinking of? As a thought experiment, what would break if we randomized the port selection for each Revision?

…

On Fri, Dec 21, 2018 at 9:11 AM Markus Thömmes ***@***.***> wrote: +1 to @dgerd <https://github.com/dgerd>. The barrier of actually configuring the PORT seems so low compared to the amount of false positives and "surprise" we could get through inferring stuff through EXPOSE that I'm also inclined to take that out of scope. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2258 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN5sdbNKgGzaDRBWDgBn4kYWX86KAks5u7RY9gaJpZM4Xt3oK> .

-- Evan Anderson <argent@google.com>

[dgerd]

If we're falling back to EXPOSE, I'd expect only a single port to be
present (or for the Revision to be in an error state).

Talked about this in person with Evan. With EXPOSE inherited from base images (i.e. FROM nginx ... EXPOSE 8080) it will be easy to accidentally have multiple ports exposed (or have a port added/removed underneath you on an image rebuild).

What would break if we randomized the port
selection for each Revision?

Most of our examples. :) But more seriously...

To rephrase my understanding of your question is: "Should we expect our users to understand they should consume $PORT, and those that are unable to consume it MUST set containerPorts?" While I think it should be best practice to consume $PORT, I don't think that randomizing the port buys us (knative) much, and doing so is guaranteed to add on-boarding friction for at least the subset of users that are already on 8080. If we got benefit in performance, scale, or user experience (past on-boarding) I think it would be a good discussion to have, but I don't see that right now.

[Fryuni]

I think there will be less friction for developers if it uses just:

1. User's explicit yaml.
2. Default port.

And that default port could be changed during the installation (only then so it would no break everything), normally set to 8080.
This way for those who use 8080 most times there will be no changes from what we have now, but for those who use another port more (like the company I work, we use 8010) will be able to set their default port and just explicitly set another one when needed.

Checking the EXPOSE on metadata looks like a hack to me. Some sort of workaround just so there is no need to explicitly set a port different from the default. If its a custom port you should explicitly customize it.

[evankanderson]

Dan convinced me that using EXPOSE is dangerous, because it is inherited from parent Dockerfiles (but the command might be changed so that nothing is listening there) in such a way that this is just as likely to cause grief as delight. In particular, a base image author can inadvertently break downstream images by changing the EXPOSE fields on a new image to add or remove entries. (Adding an entry might cause a new value to be chosen over default, or cause there to be two ports which would cause a deployment-time error; removing an entry might lead to falling back to a lower default (like 8080) in such a way that unchanged application code breaks.)

…

On Sat, Jan 5, 2019 at 8:32 PM Luiz Augusto Alves Ferraz < ***@***.***> wrote: I think there will be less friction for developers if it uses just: 1. User's explicit yaml. 2. Default port. And that default port could be changed during the installation (only then so it would no break everything), normally set to 8080. This way for those who use 8080 most times there will be no changes from what we have now, but for those who use another port more (like the company I work, we use 8010) will be able to set their default port and just explicitly set another one when needed. Checking the EXPOSE on metadata looks like a hack to me. Some sort of workaround just so there is no need to explicitly set a port different from the default. If its a custom port you should explicitly customize it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2258 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyNz--rBqzNxpzs8cP3FZSlzWk5S9sks5vAXxwgaJpZM4Xt3oK> .

-- Evan Anderson <argent@google.com>

[dgerd]

/close

It sounds like we have agreement to not proceed on EXPOSE. Going to go ahead and close this issue.

I would like to treat customizing the cluster-wide default as a new issue rather than keeping this one open. The remaining functionality from this issue was implemented in #2642.

[knative-prow-robot]

@dgerd: Closing this issue.

In response to this:

/close

It sounds like we have agreement to not proceed on EXPOSE. Going to go ahead and close this issue.

I would like to treat customizing the cluster-wide default as a new issue rather than keeping this one open. The remaining functionality from this issue was implemented in #2642.

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.