Rejecting valid Environment Variable Names

[DarqueWarrior]

Kubernetes version:

Client Version: version.Info{Major:"1", Minor:"7", GitVersion:"v1.7.0", GitCommit:"d3ada0119e776222f11ec7945e6d860061339aad", GitTreeState:"clean", BuildDate:"2017-06-29T23 15:59Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"windows/amd64"}
Server Version: version.Info{Major:"1", Minor:"6", GitVersion:"v1.6.6", GitCommit:"7fa1c1756d8bc963f1a389f4a6937dc71f08ada2", GitTreeState:"clean", BuildDate:"2017-06-16T18 21:54Z", GoVersion:"go1.7.6", Compiler:"gc", Platform:"linux/amd64"}

Environment:

• Cloud provider or hardware configuration: Azure Container Service
• OS: Windows 10
• Install tools:
• Others:

What happened:
I was invoking the following command:
kubectl run myname --image=myimage:86 --port=80 --env="ConnectionStrings:DefaultConnection=Data Source=tcp:mySqlServer,1433;Initial Catalog=myDB;User Id=myUser;Password=mypassword;"
I got errors about invalid env:
error: invalid env: ConnectionStrings:DefaultConnection=Data Source=tcp:mySqlServer

I tried this with a YAML file as well and got a better error message stating I was using invalid characters for my name. However, : is valid the only values not allowed in names is "=". To work around this I removed the : then I got an error because it split my value on "," between my server and port.

What you expected to happen:
I expected a new environment variable to be created with the following information:

• name: ConnectionStrings:DefaultConnection
• value: Data Source=tcp:mySqlServer,1433;Initial Catalog=myDB;User Id=myUser;Password=mypassword;

How to reproduce it (as minimally and precisely as possible):
Just try and set the environment variable name to something that includes a ":" and have a value that contains a ",". The splitting should not split on "," in the value of the environment variable.

Anything else we need to know:
I am using .NET Core and it allows you to create nested configurations that are built using a : in the environment variable name. I was able to use this just fine with docker run commands against my own host because docker and Linux does support the string as I originally submitted it. This only breaks because of the validation of values and parsing logic of kubectl.

[zjj2wry]

@DarqueWarrior the env key not support name: "a:b", but value can set value: "a:b".

[apelisse]

According to POSIX:

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.

Which let's me believe that we should allow "other characters".

Also, try this:

$ python
>>> import os
>>> os.environ["ConnectionStrings:DefaultConnection"] = "Data Source=tcp:mySqlServer,1433;Initial Catalog=myDB;User Id=myUser;Password=mypassword;"
>>> print os.environ["ConnectionStrings:DefaultConnection"]
Data Source=tcp:mySqlServer,1433;Initial Catalog=myDB;User Id=myUser;Password=mypassword;
>>> os.system("env | grep 'ConnectionStrings:DefaultConnection'")
ConnectionStrings:DefaultConnection=Data Source=tcp:mySqlServer,1433;Initial Catalog=myDB;User Id=myUser;Password=mypassword;
0

I don't see anything wrong there.

[pwittrock]

Fixed in 1.8

kubernetes/kubernetes@b99e571

[mengqiy]

kubernetes/kubernetes#47460 doesn't fix this issue with :.
Reopening the issue.

[mengqiy]

It seems kubectl doesn't like : in the env key.

@shiywang Do have bandwidth to verify if this is a bug in Cobra or in kubectl code?

[shiywang]

/assign

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[mengqiy]

@shiywang Any update?

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[nikhita]

/remove-lifecycle stale

[vithati]

I wanted to learn more about kubernetes and thus started investigating this issue out of interest.

@mengqiy , I found that the issue is not in Cobra but in kubectl code. Cobra is parsing arguments properly but kubectl is not allowing ":" in environment name. Allowed characters in environment are [-._a-zA-Z][-._a-zA-Z0-9]* as per this code.

Is it ok if I take up this issue and raise PR for the fix?

[apelisse]

@vithati Please, feel free to open a PR for this! Thanks (and please assign me).

[seans3]

/sig cli
/area kubectl

[seans3]

/assign @vithati

[k8s-ci-robot]

@seans3: GitHub didn't allow me to assign the following users: vithati.

Note that only kubernetes members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @vithati

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.

[seans3]

/priority P1

[bgrant0607]

Please see kubernetes/kubernetes#53201

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[seans3]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[seans3]

/remove-lifecycle stale

[pswica]

I tried resolving this here: kubernetes/kubernetes#78910

[apelisse]

There is already a proposal to fix it in kubernetes/kubernetes#69415, and it's not that easy. Though it seems like it's been abandoned. so I'd be happy to see someone pick it up

[apelisse]

See also:

• Colon character in environment variable name yields "invalid value"  kubernetes#70232
• Environment variables containing colon not allowed kubernetes#53201

[pswica]

Woah yeah it looks like this had much more history than I expected, and that I'm probably wrong haha.

Would you happen to know why my solution is not good? I basically found that line was the culprit and ran some fmt.Printlns. From there I went to a regex tester until I found something that worked. Then I tested against hack/local-up-cluster.sh and it worked like a charm.

[apelisse]

Your code is perfectly fine. The problem is with backward compatibility. If we start inserting these newly permitted object in the database, it might be a problem when you have multiple masters running at the same time (and not being updated at the same time), or if you fallback your master upgrade to a former version. Then those objects are no longer valid (this is an API change) and will be "stuck" in etcd.

[pswica]

That was an amazing explanation. Thanks a lot for taking the time to post that, I can now see that my solution was pretty naive because, in its current state, I can't imagine it being backwards compatible.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[seans3]

/remove-lifecycle stale

[liggitt]

/close
in favor of kubernetes/kubernetes#53201 (specifically coordinating in kubernetes/kubernetes#53201 (comment))

[k8s-ci-robot]

@liggitt: Closing this issue.

In response to this:

/close
in favor of kubernetes/kubernetes#53201 (specifically coordinating in kubernetes/kubernetes#53201 (comment))

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.