Fix Windows credential provider cannot find binary #120291

lzhecheng · 2023-08-31T05:55:20Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fix Windows credential provider cannot find binary. Windows credential provider binary path may have ".exe" suffix so it is better to use LookPath() to support it flexibly.

Which issue(s) this PR fixes:

Fixes #kubernetes-sigs/cloud-provider-azure#4533

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix Windows credential provider cannot find binary. Windows credential provider binary path may have ".exe" suffix so it is better to use LookPath() to support it flexibly.

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

lzhecheng · 2023-08-31T06:44:50Z

/test pull-kubernetes-node-e2e-containerd

lzhecheng · 2023-09-06T07:16:15Z

@dchen1107 @derekwaynecarr could you please take a look at the PR？Thank you.

andyzhangx · 2023-09-07T13:28:35Z

pkg/credentialprovider/plugin/plugin.go

@@ -98,7 +98,10 @@ func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string) er
 	registerMetrics()

 	for _, provider := range credentialProviderConfig.Providers {
-		pluginBin := filepath.Join(pluginBinDir, provider.Name)
+		pluginBin, err := exec.LookPath(filepath.Join(pluginBinDir, provider.Name))


could you also add comments here, and also it's better having a ut coverage for this. thanks.
btw, is this a regression? does external windows credential provider never work?

Sure.
Since external credential provider is relatively new feature I think (?) and it is also Windows (fewer users), I'm not sure if other clouds have started using it.

I added a comment there. The reason that I don't add a UT is that UT tests are still run on linux, not the OS of this change.

andyzhangx · 2023-09-12T02:37:18Z

pkg/credentialprovider/plugin/plugin.go

+		// Considering Windows binary with suffix ".exe", LookPath() helps to find the correct path.
+		pluginBin, err := exec.LookPath(filepath.Join(pluginBinDir, provider.Name))
+		if err != nil {
+			return fmt.Errorf("error finding binary path of plugin provider %s: %w", provider.Name, err)


original logic does not return error while this PR would returns error, and in L106, it's already returning error if pluginBin not found

I combined the code with the code below because LookPath calls os.Stat

Windows credential provider binary path may have ".exe" suffix so it is better to use LookPath() to support it flexibly. Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

andyzhangx

/lgtm
/kind bug
/priority important-soon
/sig cloud-provider
/area provider/azure
/triage accepted

k8s-ci-robot · 2023-09-12T03:22:32Z

LGTM label has been added.

Git tree hash: ecd127f4903ba71e2dfb5521c1e375836e1dfad9

andyzhangx · 2023-09-12T03:22:39Z

/retest

andyzhangx · 2023-09-12T03:23:48Z

@andrewsykim can you take a look at this PR? thanks.

andyzhangx · 2023-09-12T03:24:53Z

btw, I think the PR title should be support credential provider on Windows node, without this PR, it won't work on windows.

lzhecheng · 2023-09-18T04:37:46Z

@andrewsykim could you please review or refer someone to help? Thx.

lzhecheng · 2023-09-25T05:14:54Z

@dims could you please help review?

dims · 2023-09-25T12:45:37Z

/assign @nckturner @andrewsykim

lzhecheng · 2023-11-15T02:49:22Z

@nckturner @andrewsykim could you take a look? thanks

dims · 2024-01-04T13:48:18Z

/approve
/lgtm

cc @liggitt in case there were some lingering concerns around exec.LookPath (looks fine to me as this does append the full directory and the binary name itself)

k8s-ci-robot · 2024-01-04T13:48:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, dims, lzhecheng

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~~ [dims]

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

liggitt · 2024-01-04T15:35:07Z

pkg/credentialprovider/plugin/plugin.go

+		// Considering Windows binary with suffix ".exe", LookPath() helps to find the correct path.
+		// LookPath() also calls os.Stat().
+		pluginBin, err := exec.LookPath(filepath.Join(pluginBinDir, provider.Name))
+		if err != nil {
 			if os.IsNotExist(err) {


does LookPath wrap errors? do we need to call errors.Is(err, os.ErrNotExist) instead?

LookPath calls findExecutable which wraps os.Stat.
However, I think it is better to follow the suggestion in comments to use errors.Is.

@liggitt PTAL
#122600

andyzhangx · 2024-04-29T02:58:15Z

/cherrypick release-1.29

k8s-ci-robot requested review from dchen1107 and derekwaynecarr August 31, 2023 05:56

andyzhangx reviewed Sep 7, 2023

View reviewed changes

lzhecheng force-pushed the fix-credentialprovider-win-bin-path branch from 4abdc2f to bdbc87f Compare September 8, 2023 07:13

andyzhangx reviewed Sep 12, 2023

View reviewed changes

Fix Windows credential provider cannot find binary

6102357

Windows credential provider binary path may have ".exe" suffix so it is better to use LookPath() to support it flexibly. Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>

lzhecheng force-pushed the fix-credentialprovider-win-bin-path branch from bdbc87f to 6102357 Compare September 12, 2023 02:48

andyzhangx approved these changes Sep 12, 2023

View reviewed changes

k8s-ci-robot assigned andyzhangx Sep 12, 2023

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 12, 2023

k8s-ci-robot assigned andrewsykim and nckturner Sep 25, 2023

lzhecheng mentioned this pull request Nov 10, 2023

[release-1.11] Add support for OOT cred provider in ci templates kubernetes-sigs/cluster-api-provider-azure#4232

Merged

4 tasks

k8s-ci-robot assigned dims Jan 4, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 4, 2024

liggitt reviewed Jan 4, 2024

View reviewed changes

k8s-ci-robot merged commit 2efed1f into kubernetes:master Jan 4, 2024
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Jan 4, 2024

lzhecheng deleted the fix-credentialprovider-win-bin-path branch January 5, 2024 05:04

lzhecheng restored the fix-credentialprovider-win-bin-path branch January 5, 2024 05:04

lzhecheng deleted the fix-credentialprovider-win-bin-path branch January 5, 2024 05:04

This was referenced Apr 29, 2024

Automated cherry pick of #120291: Fix Windows credential provider cannot find binary Windows #124603

Open

Automated cherry pick of #120291: Fix Windows credential provider cannot find binary Windows #124604

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Windows credential provider cannot find binary #120291

Fix Windows credential provider cannot find binary #120291

lzhecheng commented Aug 31, 2023

lzhecheng commented Aug 31, 2023

lzhecheng commented Sep 6, 2023

andyzhangx Sep 7, 2023

lzhecheng Sep 8, 2023 •

edited

lzhecheng Sep 8, 2023

andyzhangx Sep 12, 2023

lzhecheng Sep 12, 2023 •

edited

andyzhangx left a comment

k8s-ci-robot commented Sep 12, 2023

andyzhangx commented Sep 12, 2023

andyzhangx commented Sep 12, 2023

andyzhangx commented Sep 12, 2023

lzhecheng commented Sep 18, 2023

lzhecheng commented Sep 25, 2023

dims commented Sep 25, 2023

lzhecheng commented Nov 15, 2023

dims commented Jan 4, 2024

k8s-ci-robot commented Jan 4, 2024

liggitt Jan 4, 2024

lzhecheng Jan 5, 2024

lzhecheng Jan 5, 2024

andyzhangx commented Apr 29, 2024

Fix Windows credential provider cannot find binary #120291

Fix Windows credential provider cannot find binary #120291

Conversation

lzhecheng commented Aug 31, 2023

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

lzhecheng commented Aug 31, 2023

lzhecheng commented Sep 6, 2023

andyzhangx Sep 7, 2023

Choose a reason for hiding this comment

lzhecheng Sep 8, 2023 • edited

Choose a reason for hiding this comment

lzhecheng Sep 8, 2023

Choose a reason for hiding this comment

andyzhangx Sep 12, 2023

Choose a reason for hiding this comment

lzhecheng Sep 12, 2023 • edited

Choose a reason for hiding this comment

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 12, 2023

andyzhangx commented Sep 12, 2023

andyzhangx commented Sep 12, 2023

andyzhangx commented Sep 12, 2023

lzhecheng commented Sep 18, 2023

lzhecheng commented Sep 25, 2023

dims commented Sep 25, 2023

lzhecheng commented Nov 15, 2023

dims commented Jan 4, 2024

k8s-ci-robot commented Jan 4, 2024

liggitt Jan 4, 2024

Choose a reason for hiding this comment

lzhecheng Jan 5, 2024

Choose a reason for hiding this comment

lzhecheng Jan 5, 2024

Choose a reason for hiding this comment

andyzhangx commented Apr 29, 2024

lzhecheng Sep 8, 2023 •

edited

lzhecheng Sep 12, 2023 •

edited