Fixed set env did not support keys with dot in it #98846

lauchokyip · 2021-02-07T03:38:02Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
Fixed the bug that cannot set an environment variable name with a dot.
To reproduce:
kubectl set env deployment/app test.on=true.
An error will be thrown error: environment variables must be of the form key=value and can only contain letters, numbers, and underscores

Which issue(s) this PR fixes:

Fixes kubernetes/kubectl#1014

Special notes for your reviewer:
/cc @rikatz
/cc @soltysh
Very similar PR #48986 <- was referring to this

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-02-07T03:38:10Z

Hi @lauchokyip. 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

lauchokyip · 2021-02-07T03:39:53Z

/cc @soltysh

rikatz · 2021-02-07T14:00:20Z

/ok-to-test
/assign

lauchokyip · 2021-02-07T20:21:09Z

/retest

rikatz · 2021-02-10T16:59:03Z

TBH I'm a bit confused of what should be the right behavior: accept or not the env var with ".".

If I try to export a variable with . in the name (like export LALA.BLABLA=1) shell will not allow me, because this is not a valid identifier.

If we allow this to be done with kubectl, can this lead any other component (kubelet, the container runtime, etc) into some wrong behavior?

I'll make some tests here

rikatz · 2021-02-10T17:00:34Z

OK now I saw the referenced PR of the past that this is strict only in shells. Will make some further tests here and let you know

lauchokyip · 2021-02-10T17:06:35Z

Yup @rikatz , I think that PR mentions K8s environment is different from shell environment. I tested in a deployment pod and added some test cases.

rikatz · 2021-02-10T17:26:26Z

@lauchokyip thank you for the PR.

I'm taking a look into the 'parseIntoEnvVar' and I guess this could be a bit improved with your PR.

As an example, what I guess could work:

Remove the case !IsValidEnvironmentArgument and let the case envSpec == "-" as the first case
On the second case (strings.Contains(envSpec, "=") there's already a split there, separating the key from the Value. There, you can instead of use the IsValidEnvironmentArgument, use something like:

if !validation.IsEnvVarName(parts[0]) [....]

This validation is being used in other parts of the code (including the PR you've referenced before), like here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/cmd/util/env_file.go#L56-L60

This way, you can:

Remove all the hasChdirPrefix function (as this is already covered in validation.IsEnvVarName, I guess
Remove the first case, and move this validation to inside the code that deals with env vars with name=var

WDYT?

lauchokyip · 2021-02-10T17:35:45Z

do we still want to check for strings.HasSuffix(envSpenc, "-")?

lauchokyip · 2021-02-10T17:40:04Z

and should I include the dependency from https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/cmd/util/env_file.go#L56-L60, or copy the code inside?

rikatz · 2021-02-16T18:24:51Z

and should I include the dependency from https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/cmd/util/env_file.go#L56-L60, or copy the code inside?

Only including the util is enough IMO.

About the suffix, I guess it would be good to keep the last case statement.

rikatz · 2021-02-16T18:32:13Z

/triage accepted

lauchokyip · 2021-02-16T22:24:45Z

/retest

rikatz · 2021-02-20T19:27:15Z

/lgtm

Thank you!

rikatz · 2021-02-20T19:31:07Z

/priority important-longterm

dougsland · 2021-02-20T20:44:36Z

/lgtm

eddiezane · 2021-03-05T04:02:42Z

staging/src/k8s.io/kubectl/pkg/cmd/set/env/env_parse.go


 // IsEnvironmentArgument checks whether a string is an environment argument, that is, whether it matches the "anycharacters=anycharacters" pattern.
 func IsEnvironmentArgument(s string) bool {
 	return argumentEnvironment.MatchString(s)
 }

-// IsValidEnvironmentArgument checks whether a string is a valid environment argument, that is, whether it matches the "wordcharacters=anycharacters" pattern. Word characters can be letters, numbers, and underscores.
-func IsValidEnvironmentArgument(s string) bool {


This is an exported function but it doesn't look like it's being used elsewhere? Probably not a breaking change?

https://cs.k8s.io/?q=IsValidEnvironmentArgument&i=nope&files=&excludeFiles=&repos=

@soltysh thoughts?

This shouldn't be an issue.

this is being removed anyway :D

Moved to call validation.IsEnvVarName which is already used by other parts of the code

seans3

Except for a small nit about import aliases, this looks good to me. I will approve this once import is updated.

seans3 · 2021-03-09T18:29:06Z

staging/src/k8s.io/kubectl/pkg/cmd/set/env/env_parse.go

@@ -23,23 +23,18 @@ import (
 	"regexp"
 	"strings"

-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"


This import alias is redundant and unnecessary. In the future, most code is aliasing this import as "corev1".

vscode does this trying to "help". Probably something we could get in the developer docs if it's not already there.

I get beat by vscode ALWAYS with the autoimport doing this :/

seans3 · 2021-03-09T18:42:23Z

/retest

lauchokyip · 2021-03-09T18:53:21Z

@seans3 Fixed

eddiezane · 2021-03-09T18:54:00Z

/lgtm

seans3

/lgtm

k8s-ci-robot · 2021-03-09T19:03:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lauchokyip, rikatz, seans3

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 files:

~~staging/src/k8s.io/kubectl/OWNERS~~ [seans3]

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

seans3 · 2021-03-09T19:03:45Z

/milestone v1.21

seans3 · 2021-03-09T19:28:33Z

/retest

k8s-ci-robot requested a review from rikatz February 7, 2021 03:38

k8s-ci-robot requested a review from soltysh February 7, 2021 03:39

k8s-ci-robot assigned rikatz Feb 7, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 7, 2021

lauchokyip force-pushed the fixsetenv branch from 18f5a76 to 0704d20 Compare February 7, 2021 17:35

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Feb 16, 2021

lauchokyip force-pushed the fixsetenv branch from 32a4486 to 44bb4d4 Compare February 16, 2021 21:30

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2021

rikatz approved these changes Feb 20, 2021

View reviewed changes

k8s-ci-robot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 20, 2021

k8s-ci-robot assigned dougsland Feb 20, 2021

eddiezane reviewed Mar 5, 2021

View reviewed changes

seans3 suggested changes Mar 9, 2021

View reviewed changes

Fixed set env did not support keys with dot in it

ab5a81c

lauchokyip force-pushed the fixsetenv branch from 44bb4d4 to ab5a81c Compare March 9, 2021 18:52

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021

k8s-ci-robot assigned eddiezane Mar 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2021

seans3 approved these changes Mar 9, 2021

View reviewed changes

k8s-ci-robot assigned seans3 Mar 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 9, 2021

k8s-ci-robot merged commit 114c656 into kubernetes:master Mar 10, 2021

lauchokyip deleted the fixsetenv branch March 14, 2021 14:25

lauchokyip mentioned this pull request Apr 9, 2021

REQUEST: New membership for lauchokyip kubernetes/org#2629

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed set env did not support keys with dot in it #98846

Fixed set env did not support keys with dot in it #98846

lauchokyip commented Feb 7, 2021 •

edited

Loading

k8s-ci-robot commented Feb 7, 2021

lauchokyip commented Feb 7, 2021

rikatz commented Feb 7, 2021

lauchokyip commented Feb 7, 2021

rikatz commented Feb 10, 2021

rikatz commented Feb 10, 2021

lauchokyip commented Feb 10, 2021

rikatz commented Feb 10, 2021

lauchokyip commented Feb 10, 2021

lauchokyip commented Feb 10, 2021

rikatz commented Feb 16, 2021

rikatz commented Feb 16, 2021

lauchokyip commented Feb 16, 2021

rikatz commented Feb 20, 2021

rikatz commented Feb 20, 2021

dougsland commented Feb 20, 2021

eddiezane Mar 5, 2021

eddiezane Mar 9, 2021

rikatz Mar 9, 2021

seans3 left a comment

seans3 Mar 9, 2021

eddiezane Mar 9, 2021

rikatz Mar 9, 2021

lauchokyip Mar 9, 2021

seans3 commented Mar 9, 2021

lauchokyip commented Mar 9, 2021

eddiezane commented Mar 9, 2021

seans3 left a comment

k8s-ci-robot commented Mar 9, 2021

seans3 commented Mar 9, 2021

seans3 commented Mar 9, 2021

Fixed set env did not support keys with dot in it #98846

Fixed set env did not support keys with dot in it #98846

Conversation

lauchokyip commented Feb 7, 2021 • edited Loading

k8s-ci-robot commented Feb 7, 2021

lauchokyip commented Feb 7, 2021

rikatz commented Feb 7, 2021

lauchokyip commented Feb 7, 2021

rikatz commented Feb 10, 2021

rikatz commented Feb 10, 2021

lauchokyip commented Feb 10, 2021

rikatz commented Feb 10, 2021

lauchokyip commented Feb 10, 2021

lauchokyip commented Feb 10, 2021

rikatz commented Feb 16, 2021

rikatz commented Feb 16, 2021

lauchokyip commented Feb 16, 2021

rikatz commented Feb 20, 2021

rikatz commented Feb 20, 2021

dougsland commented Feb 20, 2021

eddiezane Mar 5, 2021

Choose a reason for hiding this comment

eddiezane Mar 9, 2021

Choose a reason for hiding this comment

rikatz Mar 9, 2021

Choose a reason for hiding this comment

seans3 left a comment

Choose a reason for hiding this comment

seans3 Mar 9, 2021

Choose a reason for hiding this comment

eddiezane Mar 9, 2021

Choose a reason for hiding this comment

rikatz Mar 9, 2021

Choose a reason for hiding this comment

lauchokyip Mar 9, 2021

Choose a reason for hiding this comment

seans3 commented Mar 9, 2021

lauchokyip commented Mar 9, 2021

eddiezane commented Mar 9, 2021

seans3 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 9, 2021

seans3 commented Mar 9, 2021

seans3 commented Mar 9, 2021

lauchokyip commented Feb 7, 2021 •

edited

Loading