Proposal: Fail revision creation if system-specified env vars are set

[steren]

/area API
/kind proposal

I have observed developers setting the PORT environment variable.

As documented in the runtime container contract, PORT is an env var that is automatically injected into the containers and that the code should listen on.

A developers setting it probably expects that its final value will be the set value, but instead, we are overriding it. We should avoid that confusion.

Proposal: fail the creation of the revision if the developer sets the PORT env var. Return an error message saying: "The PORT environment variable is automatically set by the system"

[mattmoor]

The runtime contract states that the PORT variable will contain the value, but also indicates that the user may provide the port to listen on.

containerPort is one way we must support for h2c upgrades, but PORT is another option enumerated (IIRC) in the original discussion with @evankanderson

See https://github.com/knative/serving/blob/master/docs/runtime-contract.md#protocols-and-ports

Cc @dgerd

[steren]

Can you clarify: if I set "PORT=80" in my revision spec, HTTP requests will be sent to the value I defined (80)?

[steren]

I tested: even if I set

spec:
  container:
    env:
    - name: PORT
      value: '80'

The value of $PORT inside my container is 8080.

So my proposal is still valid: it would help setting developer expectations if we rejected revisions that are setting the PORT env var.

[markusthoemmes]

I think @mattmoor is saying that we could also check for the PORT environment variable to be set (if it does not conflict with containerPort) and use that as the user-defined port. We can only support one transport there though (http1 likely?) and we'd need to implement validation that containerPort and PORT do not contradict each other.

@steren based on your observations from users, does that sound useful? Seems like users naturally try to set these environment variables?

[dgerd]

Here is some of the original discussion on the PR for containerPort: #2642 (comment)

The current behavior is that both the user set and system set environment variables are available through the API. The system set environment variable is where the Queue Proxy is actually forwarding and is what is set within the container.

fail the creation of the revision if the developer sets the PORT env var.

If we want to go down this route we should do so for all system specified environment variables as the same behavior will occur. Note that this may be potentially tricky as we do not necessarily know if the SHOULD or MAY environment variables will be set by the controller when we receive the request in the webhook ( https://github.com/knative/serving/blob/master/docs/runtime-contract.md#process ).

we could also check for the PORT environment variable to be set (if it does not conflict with containerPort) and use that as the user-defined port.

I worry about having two places in the API to set the same thing and that it may cause more confusion. I don't imagine us handling cases where the environment variable is populated within the container or through a ConfigMap so there still will be cases where PORT will be overriden by the system.

[mattmoor]

I was thinking about the expected behavior in #2258, but that doesn't specify this. I don't feel strongly either way, just trying to make sure prior discussions are surfaced here.

[evankanderson]

Re #3328 (comment): I think we can avoid the confusion by having an order of precedence and/or failing configurations which request conflicting information. My personal preference would be to try to accept the user intent as long as it's clear, rather than requiring that users spell the request one particular way (yes, this is moving from API to UX considerations in my mind).

[steren]

@dgerd I agree that if we reject PORT, we should also reject the other system set env vars.

I am not sure we should interpret users setting PORT to a given number as an intent to send requests to this port number.
In term of UX, we can totally imagine clients (in particular UIs) asking very explicitely for a container port. This would be more explicit than relying on one particular special env var.

I'd like to focus this particular issue on the fact that we do not reject env vars that conflict with system set env vars, like PORT, can Knative reject them (short term)?

[dgerd]

@steren

I am curious as to why we are seeing users accidentally setting PORT in the first place. Is our documentation unclear that this is set for you by the system? Is the user testing the application locally and getting used to passing env through on the docker command line? Is setting PORT a best practice of another system they are used to using? Are they using the environment variable for another purpose?

Depending on the reason we are seeing users set the PORT I think we may end up with different a solution.

Currently environment variables can be set on a container through:
1.) Container Image Parent
2.) Dockerfile ENV statements ( https://docs.docker.com/engine/reference/builder/#env )
3.) Using "envFrom" and a ConfigMap ( https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables )
4.) Using "env" to specify key/value

The only method that we could easily catch in the webhook is the fourth method as the content will be present in the request. It may be easy to dismiss the first two methods as something we do not want to cover, but the third method is a very common way to configure environment variables. Covering only the fourth method would create inconsistency with our validation which to me is worse than our existing behavior of overriding.

[steren]

Users do not read docs (and thus try to use define PORT like they would do on other systems)
When they do, we tell them to "listen on $PORT", so some of them believe they have to define "PORT"

My request only applies to when users set PORT in the revision spec.
I think it's better to catch some errors than none at all.

Clients can also help to prevent this mistake.

[evankanderson]

My understanding is that $PORT in the container will be set preferentially in the following order:

1. Update by code inside the container (e.g. a shell script)
2. Explicit setting via either env or envFrom (and these two should be mutually-exclusive)
3. Dockerfile ENV statements
4. Container Image Parent ENV statements

We can treat 1. as equivalent to code which doesn't bother to read $PORT, since there's no way to read the env var out from a running process without its cooperation (that I'm aware of).

For 2., I think there's benefit in recognizing the env version and validating it as another way of setting container.ports[0]. (Bonus: if env[PORT] and container.port[0] mismatch, we should be able to provide a clear error.) As part of this, we should probably prevent users from using valueRef for PORT, because this will almost certainly lead to tears. This is somewhat harder to do with envFrom, as it defines a mapping across the keys of a ConfigMap or Secret onto environment variables. (In any case, the env list trumps the envFrom list.)

For 3. and 4., these environment variables will already be ignored by the platform by our setting of the PORT in env, so they are already being overridden.

The two properties I think are important are:

1. $PORT inside the container corresponds with container.ports[0]
2. That customers are easily able to debug cases where the routing infrastructure isn't handing off to their container.

The simplest debugging of 2. is when there is no bug, because it's working. Next best is some sort of obvious warning that something is wrong. Next best is something obvious in the logs warning you. Worst is scratching your head and adding more instrumentation to your container which ends up not getting called. It's with that perspective that I suggest using env[PORT] to default the container.ports[0] value if it's not already set, as it will move a set of Java, PHP, etc developers into the best case (working) from one of the worse debugging cases.