Be strict about env vars reserved by the runtime contract.

[mattmoor]

Add validation analogous to the "reserved paths" validation for
volume mounts, which rejects env vars that take a name that has
been specified as part of the runtime contract.

Fixes: #3328

/hold

I want to make sure @dgerd is on board with this, given the back-and-forth in the linked issue.

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• pkg/apis/OWNERS [mattmoor]
• pkg/reconciler/OWNERS [mattmoor]

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

[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/k8s_validation.go	98.6%	99.3%	0.7

[vagababov]

/lgtm

[dgerd]

I am okay with this. I do think this is the right timing to add stricter validation as the longer we wait the more breaking this becomes. I also like that it covers all environment variables that we set so we get a more consistent experience rather than just PORT. This won't catch all of the error cases (i.e. envFrom), but our default controller behavior seems sane when there is a conflict.

Given the possibility of adding additional "K_[A-Z]*" environment variables in the future, I do think we should potentially be more aggressive here and cover the regex space. We do this already similar things for the "name" field of port, and this will prevent new knative environment variables from being breaking changes.

[mattmoor]

As discussed in person, I think we should reserve KNATIVE_ vs. K_, but I think that warrants further discussion and follow-up.

[mattmoor]

/hold cancel