Appended OS's environment variables to the ones configured in Credent… #103231

n4j · 2021-06-26T09:36:36Z

Appended OS's environment variables with the ones provided in the Credential Provider Config file
Wrote UnitTest cases to validate the above behaviour

What type of PR is this?

/kind bug

What this PR does / why we need it:

According to the behaviour described in the kubelete-credential-provider documentation, the environment variables provided in the config file should be appended with the OS env vars.

However, this seems to be a missed out case and credential providers may fail to execute if $PATH and $HOME are missing.

This bug was verified for ecr-credential-provider

Which issue(s) this PR fixes:

Fixes #102750

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed an issue which didn't append OS's environment variables with the one provided in Credential Provider Config file, which may lead to failed execution of external credential provider binary. 
See https://github.com/kubernetes/kubernetes/issues/102750

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

k8s-ci-robot · 2021-06-26T09:36:44Z

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

MadhavJivrajani · 2021-06-26T09:48:18Z

/ok-to-test

n4j · 2021-06-26T10:01:26Z

/sig node

n4j · 2021-06-26T11:40:49Z

/test pull-kubernetes-integration

pkg/credentialprovider/plugin/config.go

k8s-ci-robot · 2021-06-26T13:21:04Z

@n4j: The label triage/accepted cannot be applied. Only GitHub organization members can add the label.

In response to this:

/triage accepted

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.

pkg/credentialprovider/plugin/config.go

n4j · 2021-06-28T13:48:16Z

@thockin @smarterclayton Can anyone of you guys review this PR?

n4j · 2021-06-28T13:51:12Z

@adisky Can you accept the triage on this PR?

n4j · 2021-06-28T13:51:46Z

/priority important-soon

adisky · 2021-06-28T14:25:27Z

/triage accepted

n4j · 2021-07-23T04:58:03Z

/test pull-kubernetes-e2e-kind-ipv6

liggitt · 2021-07-23T17:37:54Z

/lgtm
/approve

k8s-ci-robot · 2021-07-23T17:38:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, n4j

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:

~~pkg/credentialprovider/OWNERS~~ [liggitt]

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

n4j · 2021-07-23T18:25:42Z

I'd consider this a bug fix since the API docs say the kubelet env vars are appended, so I would like to see this land in v1.22, if not it should be cherry-picked anyways.

@liggitt How do I get this merged in v1.22 release?

liggitt · 2021-07-23T18:28:57Z

this can wait until master reopens for 1.23, and be picked to release-1.22 for 1.22.1 if needed

n4j · 2021-07-27T15:31:15Z

/test all

n4j · 2021-07-27T15:32:14Z

/meow caturday

k8s-ci-robot · 2021-07-27T15:32:16Z

@n4j:

In response to this:

/meow caturday

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.

n4j · 2021-07-27T16:20:43Z

/retest

n4j · 2021-07-31T01:38:38Z

/test all

n4j · 2021-07-31T01:48:25Z

/retest

liggitt · 2021-08-04T12:56:09Z

pkg/credentialprovider/plugin/plugin.go

+	mergedEnvVars := sysEnvVars
+	for _, credProviderVar := range credProviderVars {
+		mergedEnvVars = append(mergedEnvVars, credProviderVar)
+	}
+	return mergedEnvVars


this is effectively

return append(sysEnvVars, credProviderVars...)

I don't think we need a separate method to accomplish the same thing as the built-in append method.

liggitt · 2021-08-04T13:02:15Z

pkg/credentialprovider/plugin/plugin.go

+	var configEnvVars []string
+	for _, v := range e.envVars {
+		configEnvVars = append(configEnvVars, fmt.Sprintf("%s=%s", v.Name, v.Value))
 	}

+	// Append current system environment variables, to the ones configured in the
+	// credential provider file. Failing to do so may result in unsuccessful execution
+	// of the provider binary, see https://github.com/kubernetes/kubernetes/issues/102750
+	// also, this behaviour is inline with Credential Provider Config spec
+	cmd.Env = mergeEnvVars(e.environ(), configEnvVars)


The unit test is only exercising mergeEnvVars, not the inputs to it given the environ() and e.envVars values.

Computing the env like this:

cmd.Env = e.computeEnv()

would let us put the entire implementation calling environ() call and appending e.envVars in a unit testable location.

n4j · 2021-08-05T04:15:55Z

@liggitt Since this PR was approved and the branch for 1.23 opened up it was auto merged, I can open a separate PR to fix the comments.

Thoughts?

liggitt · 2021-08-05T04:51:18Z

Sure, a follow up is fine

brandond · 2021-08-31T19:00:16Z

I'm having a hard time following the breadcrumbs here - did this ever get cleaned up and/or backported to 1.21 and 1.22?

brandond · 2021-09-09T23:26:06Z

@liggitt @n4j this is still broken on the older release branches, correct?

liggitt · 2021-09-10T13:15:16Z

yeah, this is only resolved in 1.23

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 26, 2021

k8s-ci-robot requested review from andrewsykim and dims June 26, 2021 09:37

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 Jun 26, 2021

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 26, 2021

MorrisLaw reviewed Jun 26, 2021

View reviewed changes

pkg/credentialprovider/plugin/config.go Outdated Show resolved Hide resolved

n4j requested a review from MorrisLaw June 26, 2021 12:20

adisky reviewed Jun 28, 2021

View reviewed changes

pkg/credentialprovider/plugin/config.go Outdated Show resolved Hide resolved

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jun 28, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 28, 2021

k8s-ci-robot assigned liggitt Jul 23, 2021

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

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

n4j moved this from Needs Reviewer to Done in SIG Node PR Triage Jul 23, 2021

liggitt added this to the v1.23 milestone Jul 23, 2021

liggitt moved this from Needs Triage to In Review in SIG Auth Old Aug 2, 2021

ehashman moved this from Done to Needs Approver in SIG Node PR Triage Aug 2, 2021

liggitt reviewed Aug 4, 2021

View reviewed changes

k8s-ci-robot merged commit d1479ea into kubernetes:master Aug 5, 2021

SIG Node PR Triage automation moved this from Needs Approver to Done Aug 5, 2021

SIG Auth Old automation moved this from In Review to Closed / Done Aug 5, 2021

brandond mentioned this pull request Sep 9, 2021

Validate k3s kubelet image-credential-provider support k3s-io/k3s#3463

Closed

This was referenced Sep 14, 2021

Automated cherry pick of #103231: Overlaid OS's environment variables with the ones specified #105011

Closed

Automated cherry pick of #103231: Overlaid OS's environment variables with the ones specified #105013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Appended OS's environment variables to the ones configured in Credent… #103231

Appended OS's environment variables to the ones configured in Credent… #103231

n4j commented Jun 26, 2021 •

edited

k8s-ci-robot commented Jun 26, 2021

MadhavJivrajani commented Jun 26, 2021

n4j commented Jun 26, 2021

n4j commented Jun 26, 2021

k8s-ci-robot commented Jun 26, 2021

n4j commented Jun 28, 2021

n4j commented Jun 28, 2021

n4j commented Jun 28, 2021

adisky commented Jun 28, 2021

n4j commented Jul 23, 2021

liggitt commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

n4j commented Jul 23, 2021

liggitt commented Jul 23, 2021

n4j commented Jul 27, 2021

n4j commented Jul 27, 2021

k8s-ci-robot commented Jul 27, 2021

n4j commented Jul 27, 2021

n4j commented Jul 31, 2021

n4j commented Jul 31, 2021

liggitt Aug 4, 2021

liggitt Aug 4, 2021

n4j commented Aug 5, 2021

liggitt commented Aug 5, 2021

brandond commented Aug 31, 2021

brandond commented Sep 9, 2021

liggitt commented Sep 10, 2021

Appended OS's environment variables to the ones configured in Credent… #103231

Appended OS's environment variables to the ones configured in Credent… #103231

Conversation

n4j commented Jun 26, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Jun 26, 2021

MadhavJivrajani commented Jun 26, 2021

n4j commented Jun 26, 2021

n4j commented Jun 26, 2021

k8s-ci-robot commented Jun 26, 2021

n4j commented Jun 28, 2021

n4j commented Jun 28, 2021

n4j commented Jun 28, 2021

adisky commented Jun 28, 2021

n4j commented Jul 23, 2021

liggitt commented Jul 23, 2021

k8s-ci-robot commented Jul 23, 2021

n4j commented Jul 23, 2021

liggitt commented Jul 23, 2021

n4j commented Jul 27, 2021

n4j commented Jul 27, 2021

k8s-ci-robot commented Jul 27, 2021

n4j commented Jul 27, 2021

n4j commented Jul 31, 2021

n4j commented Jul 31, 2021

liggitt Aug 4, 2021

Choose a reason for hiding this comment

liggitt Aug 4, 2021

Choose a reason for hiding this comment

n4j commented Aug 5, 2021

liggitt commented Aug 5, 2021

brandond commented Aug 31, 2021

brandond commented Sep 9, 2021

liggitt commented Sep 10, 2021

n4j commented Jun 26, 2021 •

edited