-
Notifications
You must be signed in to change notification settings - Fork 38.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Windows credential provider cannot find binary #120291
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,8 +98,10 @@ func RegisterCredentialProviderPlugins(pluginConfigFile, pluginBinDir string) er | |
registerMetrics() | ||
|
||
for _, provider := range credentialProviderConfig.Providers { | ||
pluginBin := filepath.Join(pluginBinDir, provider.Name) | ||
if _, err := os.Stat(pluginBin); err != nil { | ||
// 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does LookPath wrap errors? do we need to call There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return fmt.Errorf("plugin binary executable %s did not exist", pluginBin) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.