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.

Sign up for GitHub

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

Jump to bottom

Added support for colon (':') in environment variable name #59599

Closed
wants to merge 2 commits into from
Closed

Added support for colon (':') in environment variable name #59599

wants to merge 2 commits into from

Conversation

bsteciuk
Copy link
Contributor

@bsteciuk bsteciuk commented Feb 8, 2018 •
edited
Loading

What this PR does / why we need it:
This PR adds support for including a colon : in environment variable names. A number of .Net applications/libraries use : in their environment variable names.
Which issue(s) this PR fixes:
Fixes #53201

Special notes for your reviewer:

Release note:

Add support for `:` character in environment variable names.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2018
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 8, 2018
@bsteciuk
Copy link
Contributor Author

bsteciuk commented Feb 9, 2018 •
edited
Loading

/sig windows
/cc @michmike

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Feb 9, 2018
michmike
michmike reviewed Feb 9, 2018
View reviewed changes
staging/src/k8s.io/apimachinery/pkg/util/validation/validation.go Outdated
@@ -302,7 +302,7 @@ func IsHTTPHeaderName(value string) []string {
return nil
}

const envVarNameFmt = "[-._a-zA-Z][-._a-zA-Z0-9]*"
const envVarNameFmt = "[-:._a-zA-Z][-:._a-zA-Z0-9]*"
const envVarNameFmtErrMsg string = "a valid environment variable name must consist of alphabetic characters, digits, '_', '-', or '.', and must not start with a digit"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we also change the message to include the fact that we now support : ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Updated.

@michmike
Copy link
Contributor

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bsteciuk, michmike
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers: liggitt, vishh

Assign the PR to them by writing /assign @liggitt @vishh in a comment when ready.

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 OWNERS Files:

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

@bsteciuk
Copy link
Contributor Author

/assign @liggitt @vishh

@bsteciuk
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@k8s-ci-robot
Copy link
Contributor

@bsteciuk: you can't request testing unless you are a kubernetes member.

In response to this:

/test pull-kubernetes-e2e-kops-aws

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.

@jdumars
Copy link
Member

jdumars commented Feb 13, 2018

/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 Feb 13, 2018
@PatrickLang
Copy link
Contributor

PatrickLang commented Feb 13, 2018 •
edited
Loading

Should we consider this for 1.9? Does anyone need that before 1.10 is released?

NM - nobody in SIG-Windows meeting needed it for 1.9

@jdumars
Copy link
Member

jdumars commented Feb 13, 2018

/test pull-kubernetes-e2e-kops-aws

@jdumars
Copy link
Member

jdumars commented Feb 16, 2018

@bsteciuk you're going to need to contact folks from the OWNERS files listed above and get approvals.

@bsteciuk
Copy link
Contributor Author

/cc @liggitt @vishh Could you have a look at this for approval?

@k8s-github-robot
Copy link

/test all

Tests are more than 96 hours old. Re-running tests.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 22, 2018 •
edited
Loading

@bsteciuk: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-verify 93329d4 link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

kow3ns
kow3ns suggested changes Feb 27, 2018
View reviewed changes
Copy link
Member

@kow3ns kow3ns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should do this. In Linux ":" is a path separator and completely invalid to use in an environment variable name. In .NET Core you use "__" in place of ":" in order get the desired configuration functionality to work as documented

@timoreimann
Copy link
Contributor

@kow3ns FWIW, the argument brought forward in #48986 to support relaxing the rules for dots and dashes was that the mentioned restrictions seemingly only apply to shell contexts; general application usages are supposed to be handled more liberally according to the standard (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. Uppercase and lowercase letters shall retain their unique identities and shall not be folded together. The name space of environment variable names containing lowercase letters is reserved for applications. Applications can define any environment variables with names from this name space without modifying the behavior of the standard utilities.

The referenced Portable Character Set lists the colon.

See also the links I noted down in that PR's description.

@bsteciuk
Copy link
Contributor Author

bsteciuk commented Mar 6, 2018

Closing due to response and ticket close of #53201, added docs PR to explain the issue and it's workaround kubernetes/website/pull/7657 .

@bsteciuk bsteciuk closed this Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kow3ns kow3ns kow3ns requested changes

@michmike michmike michmike approved these changes

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@dims dims Awaiting requested review from dims

@mikedanese mikedanese Awaiting requested review from mikedanese

@pwittrock pwittrock Awaiting requested review from pwittrock

Assignees

@liggitt liggitt

@michmike michmike

@vishh vishh

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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/windows Categorizes an issue or PR as relevant to SIG Windows. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Environment variables containing colon not allowed
10 participants
@bsteciuk @michmike @k8s-ci-robot @jdumars @PatrickLang @k8s-github-robot @timoreimann @kow3ns @liggitt @vishh