Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Relax restrictions on environment variable names. #48986

Conversation

timoreimann
Copy link
Contributor

@timoreimann timoreimann commented Jul 15, 2017

Fixes #2707

The POSIX standard restricts environment variable names to uppercase letters, digits, and the underscore character in shell contexts only. For generic application usage, it is stated that all other characters shall be tolerated. (Reference here, my prose reasoning here.)

This change relaxes the rules to some degree. Namely, we stop requiring environment variable names to be strict C_IDENTIFIERS and start permitting lowercase, dot, and dash characters.

Public container images using environment variable names beyond the shell-only context can benefit from this relaxation. Elasticsearch is one popular example.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 15, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Jul 15, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @timoreimann. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 15, 2017
@timoreimann
Copy link
Contributor Author

/cc @mtaufen

@resouer
Copy link
Contributor

resouer commented Jul 16, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 16, 2017
@liggitt
Copy link
Member

liggitt commented Jul 16, 2017

cc @kubernetes/api-reviewers

@smarterclayton
Copy link
Contributor

So this makes env vars potentially unbounded? Special characters? Equals characters? Unicode? The null terminator?

I don't think we can allow = as that would alter how values are interpreted.

I'm not sure I'd start with "no restrictions" as the next step. I'd probably prefer to have a set of concrete additions, each backed by a concrete use case.

@redbaron
Copy link
Contributor

@timoreimann , thank you very much doing it, it was a long standing headache for us.

@smarterclayton it is not for kubernetes to decide what is allowed and what is not, it should just pass whatever user asked for to an application.

> env - '{}&&!!55=__=1' /bin/bash -c 'printenv'
PWD=/tmp
SHLVL=1
{}&&!!55=__=1
_=/usr/bin/printenv

@timoreimann
Copy link
Contributor Author

timoreimann commented Jul 16, 2017

@smarterclayton yes, the environment variable namespace would be unbound in order to be in line with the standards recommendation I linked to previously. For the sake of convenience, here's the relevant section (emphasis by me):

Environment variable names used by the utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001 consist solely of uppercase letters, digits, and the '_' (underscore) from the characters defined in Portable Character Set and do not begin with a digit. Other characters may be permitted by an implementation; applications shall tolerate the presence of such names.

Reading the section again, I realize I might have misinterpreted the passage: It looks to me now that those other characters which we should tolerate are limited to what the Portable Character Set gives. This would at least rule out unicode characters and maybe more obscure special characters.

Although one could argue that allowing the equal character simply moves the responsibility for passing non-ambiguous values onto the user, I can see how that could become a source of confusion. Keeping it on the black list may be the smarter approach indeed.

Regarding how to approach looser restrictions: My rationale was to solve this matter "once and for all". A minimalistic relaxation could lead to further feature requests and discussions as people start to use other characters not supported by Kubernetes. For instance, I think it's not too unrealistic that slashes could be employed in the future as a separation / hierarchy-building means.

The other argument is that IMHO Kubernetes should try to follow official POSIX recommendations, or otherwise have good reasons not to (the equal character being one such).

I can see two alternatives to accepting the PR in its current form:

  1. Only add the dot character to the set of permissible characters, which is what dot character in environment variable name yields "invalid value" #2707 is strictly about.
  2. Pick a reasonable sub-set from the Portable Character. All visible characters could be one such sub-set.

WDYT?

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 16, 2017
@timoreimann
Copy link
Contributor Author

/test all

@smarterclayton
Copy link
Contributor

smarterclayton commented Jul 16, 2017 via email

@timoreimann
Copy link
Contributor Author

Alright, I'll simplify the PR accordingly.

I scanned through the open issues containing "environment variable" and only found #16863 which is about supporting dashes. Tim Hockin seems to have approved the feature request already.

I can't really tell if there are legitimate yet rejected requests among the closed issues though since there are just too many of them, and Github search seems way too fuzzy (or my search-fu is weak).

@timoreimann timoreimann changed the title Allow arbitrary environment variable names. Allow environment variable names containing dots. Jul 16, 2017
@liggitt
Copy link
Member

liggitt commented Jul 16, 2017

Do nodes still run validation on pods? If so, loosening validation means a version skewed node couldn't run a pod using an envvar name containing a dot or dash for a couple releases (we have to consider nodes up to two releases older than the API server)

@liggitt
Copy link
Member

liggitt commented Jul 17, 2017

Dash and dot seem like the most reasonable expansions if we're able to work through the impact of loosening validation

@timoreimann
Copy link
Contributor Author

timoreimann commented Jul 17, 2017

@liggitt kubelet checks whether pod environmental variables comply with the C_IDENTIFIER naming scheme. Is that what you are referring to when talking about node version skew?

How would we go about phasing in the relaxation two releases from now? Would we just announce the upcoming deprecation on the next release and only make the code change in 1.10? Or can we update the code today already with some kind of version guard?

Does the version compatibility guarantee only refer to the validation on the node, or do we have to make sure it covers all API operations?

Finally, do I understand correctly that we won't be able to allow dots and dashes until those two compatibility releases have elapsed?

Sorry for the tons of questions -- doing a backwards compatible feature is a new thing for me in Kubernetes.

@redbaron
Copy link
Contributor

We can't add "all" because we would never be able to restrict it in future versions. But we can grow.

@smarterclayton , this exact thinking "let's restrict, then we can grow" lead to this PR and discussion and substantial amount of pain for users, didn't it? And now you are suggesting to repeat this approach, what for? So far problem is exactly opposite, k8s restricted it so much, that not every app can be run in kube, so lets solve exactly it. Problem is not that "." is not allowed, problem is that k8s tries to censor env var names, while it is not in the business of doing so.

@timoreimann , paragraph you are quoting is relevant to "utilities in the Shell and Utilities volume of IEEE Std 1003.1-2001" only. Which is

Definitions for a standard source code-level interface to command interpretation services (a "shell") and common utility programs for application programs

So if kube restricted itself to be running those utilities only, it would be a valid restriction, but kube meant to run arbitrary applications, therefore it should allow any env vars to be passed to an app, as defined by the standard.

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 21, 2017
@timoreimann timoreimann changed the title Allow environment variable names containing dots. Allow environment variable names to contain dots and dashes. Jul 21, 2017
@timoreimann
Copy link
Contributor Author

@smarterclayton @liggitt I have reverted the PR to the previous state and extended the environment variable name validation logic to just tolerate dots and dashes (while keeping everything else unchanged).

Please take a look again and let me know what else needs to be done. Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton, timoreimann

Associated issue: 2707

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@timoreimann
Copy link
Contributor Author

/retest

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 50208, 50259, 49702, 50267, 48986)

k8s-github-robot pushed a commit that referenced this pull request Aug 8, 2017
…ictions

Automatic merge from submit-queue (batch tested with PRs 50208, 50259, 49702, 50267, 48986)

Relax restrictions on environment variable names.

Fixes #2707

The POSIX standard restricts environment variable names to uppercase letters, digits, and the underscore character in shell contexts only. For generic application usage, it is stated that all other characters shall be tolerated. (Reference [here](http://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html), my prose reasoning [here](#2707 (comment)).)

This change relaxes the rules to some degree. Namely, we stop requiring environment variable names to be strict `C_IDENTIFIERS` and start permitting lowercase, dot, and dash characters.

Public container images using environment variable names beyond the shell-only context can benefit from this relaxation. Elasticsearch is one popular example.
@k8s-github-robot k8s-github-robot merged commit 243e655 into kubernetes:master Aug 8, 2017
@timoreimann timoreimann deleted the relax-env-var-naming-restrictions branch August 8, 2017 12:10
@liuzhen9327
Copy link

liuzhen9327 commented Sep 13, 2017

@timoreimann
hi,timoreimann
I still have a problem, can you help me?
#52381

wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 11, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 11, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 11, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 11, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 11, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 15, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 15, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 15, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 15, 2018
wozniakjan pushed a commit to wozniakjan/origin that referenced this pull request May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.