Added ':' in environment variable name valid characters list

[vithati]

What this PR does / why we need it:
This PR changes allows ':' in environment variable name for the containers. This is bug fix of issue
kubernetes/kubectl#47
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubectl#47

Special notes for your reviewer:

Release note:

Relaxed restrictions on environment variable name characters. A valid environment variable name should not start with a digit. Older version of Kubelet ignores the environment variable names having other than [-._a-zA-Z0-9] characters and using these variables names will prevent apiserver rollback to older versions.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

[vithati]

/sig cli
/area kubectl
/assign @apelisse

[vithati]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

I have signed CLA.

[apelisse]

/ok-to-test

You need to sign the CLA if you want your code to be merged.

[apelisse]

I think this is relaxing (rather than tightening) the behavior, so it shouldn't be a compatibility problem.
I also think it's POSIX compliant (see here).

We've had some problems using colons in some arguments in the past, can you maybe add a test in test/cmd?

Also, I suspect we might want to relax the regex even more, since I suspect this won't be enough to fix the issue you're trying to fix (the example does include a whitespace).

[jennybuckley]

/cc @kubernetes/api-reviewers

[lavalamp]

Rollback is unsafe as soon as someone makes a env named with a ":".

[apelisse]

That's a good point. I think kubectl shouldn't be setting the limit of what can be done for environment variables. Maybe the constraint is more relaxed on the server?

[apelisse]

Oh, OK no, this is not just for kubectl, this is changing the apimachinery check.

[lavalamp]

Yes, this is an API change.

…

On Thu, Oct 4, 2018 at 2:25 PM Antoine Pelisse ***@***.***> wrote: Oh, OK no, this is not just for kubectl, this is changing the apimachinery check. — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#69415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAngls3vpaeDFFXWNFDKQNzCFMMxyvRGks5uhnzbgaJpZM4XHqxe> .

[apelisse]

Rollback is unsafe as soon as someone makes a env named with a ":".

is this something we can't accept, or a release-note might be OK?

[thockin]

We have relaxed this in the past (it was once very strict). This is a POSIX compatible change though.

This is strictly rollback-unsafe, but I'd say it's relatively low-risk. How do we want to manage these? Do we say no to this forever? Do we replace Env with POSIXEnv?

If we do this, we should also update the comments in pkg/apis/core/types.go and staging/src/k8s.io/api/core/v1/types.go

[apelisse]

This is strictly rollback-unsafe, but I'd say it's relatively low-risk.

I'd go with this, strongly suggest in the release note to delay using this variables (which people can't use today anyway) before they are confident they won't have to fallback their apiserver.

[lavalamp]

That works for me, fix the release note & I'll approve.

(also looks like CLA needs signing)

[apelisse]

Things that need to be fixed on this PR:

• Sign the CLA
• Update the release note to indicate that starting to use these variable names will prevent further roll-backs
• Update documentation, since keys don't have to be C_IDENTIFIERs anymore:
  • https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L1655
  • https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/core/types.go#L1932
  • https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L1748
  • https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L1831
  • https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/api/core/v1/types.go#L2065

[vithati]

I signed it

[thockin]

@bgrant0607 I meant that 1.x release would tolerate and use any extended values it finds in etcd, but would not allow use of extended values in API mutations. I.e. if I upgrade to 1.y and rollback to 1.x or if I have version skew it will be OK.

Agree re: kubelet

[liggitt]

I'd envision a rollout like this:

In version X:

1. handle the expanded values, and tolerate persisted objects with the expanded values:
   1a: update validation to validate the expanded values
   1b: update consumers of the field to handle the expanded values
   1c: allow an update of an object already using the expanded values to continue using them
2. prevent new use of the expanded values:
   2a: prevent creation of a new object using the expanded values
   2b: prevent an update of an object not yet using the expanded values from starting to use them

In some later version, the restrictions on create and update (2a,2b) can be removed.

• This should be done no earlier than X+1 (so a skewed HA control plane does not consider persisted data invalid, and single version control plane rollback is safe).
• This could require a longer waiting period, depending on the consumers that were updated in 1a, what behavior they had prior to being updated, and what level of skew is supported.
  • Did they skip the new data in a safe way? (e.g. not set the new envvar)
  • Did they skip the new data in an unsafe way? (e.g. format a volume as a filesystem instead of a block device... if so, this is probably a sign the API change is not a safe one to make at all)
  • Did they hard fail? Could require waiting until the oldest supported version of the consumer is version X (when the new data was handled), or could be the desired behavior (block incorrect use of objects containing the new data)

This is the same process we have to follow when allowing a new enum value (e.g. adding "SCTP" as a valid port protocol).

In this case, it appears kubelets do not validate envvar names coming from the API, and only warn on invalid envvar names resulting from secret/configmap -> envvar mappings. That means we could relax validation in 1.14, and remove create/update restrictions in 1.15.

[thockin]

@vithati any appetite for trying to get this into 1.14 ?

[vithati]

@vithati any appetite for trying to get this into 1.14 ?

@thockin, I can't make it for 1.14. I will work on this for 1.15.

[alex-slynko]

Hi @vithati

Is there any news about the PR?
We tried to use the variable with : to configure npm registry for building image, but it failed. We have implemented a workaround, but it would be nice to have a proper fix.

[spiffxp]

/priority awaiting-more-evidence

[vithati]

I am not able to get time to work on the changes. Can someone take this up?

[spiffxp]

/remove-area conformance

[hpandeycodeit]

@vithati I would like to resume working on this PR. Let me know if that's okay with you.

[vithati]

@vithati I would like to resume working on this PR. Let me know if that's okay with you.

@vithati I would like to resume working on this PR. Let me know if that's okay with you.

Thanks, you can take it up.

[liggitt]

/close

in favor of #83032

[k8s-ci-robot]

@liggitt: Closed this PR.

In response to this:

/close

in favor of #83032

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.