Add more helpful message on plugin setup failure #223
Conversation
The current error message shown when a plugin does not respond correctly can be obtuse to the end user of, for example, Vault. Here, we add potential reasons why the plugin did not respond correctly, and some additional debugging information (provided as a best effort) including the CPU architecture that the plugin was compiled for, the current CPU architecture, and the permissions of the plugin. Hopefully this will help users diagnose why their plugin is not loading. We also added a `testdata/` directory that is optionally populated with executables in various formats to help test the additional debugging information. The binaries are over 1 MB each though, so are not checked in, and the test will be skipped if they have not been compiled.
Co-authored-by: Calvin Leung Huang <1883212+calvn@users.noreply.github.com>
Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
Co-authored-by: Tom Proctor <tomhjp@users.noreply.github.com>
| "Unrecognized remote plugin message: %s\n\n"+ | ||
| "This usually means that the plugin is either invalid or simply\n"+ | ||
| "needs to be recompiled to support the latest protocol.", line) | ||
| err = fmt.Errorf(unrecognizedRemotePluginMessage, line, additionalNotesAboutCommand(cmd.Path)) |
There was a problem hiding this comment.
Open question: Given the scope of usage of this library, and the fact we have a levelled logger available, would it potentially make sense to log the additional notes to debug level instead, and include a note about the availability of debug logs in the error message? I can definitely see the appeal of just having all the information available right there in the error message, and I can also see the possibility for grumbles about verboseness (which I personally don't sympathise with too much), so I'm genuinely undecided.
There was a problem hiding this comment.
I think by returning an error message, we give the caller the option to log and at what level. They certainly might perform an action based on the remote plugin message in the first line. I think I prefer it as is.
|
Thanks! |
| notes += "\nAdditional notes about plugin:\n" | ||
| notes += fmt.Sprintf(" Path: %s\n", path) | ||
| notes += fmt.Sprintf(" Mode: %s\n", stat.Mode()) | ||
| statT, ok := stat.Sys().(*syscall.Stat_t) |
There was a problem hiding this comment.
Hi,
this does not compile on Windows:
Error: C:\Users\runneradmin\go\pkg\mod\github.com\hashicorp\go-plugin@v1.4.7\client.go:512:36: undefined: syscall.Stat_t
https://github.com/drakkan/sftpgo/actions/runs/3641265803/jobs/6146996350
The current error message shown when a plugin does not respond correctly can be obtuse to the end user of, for example, Vault.
Here, we add potential reasons why the plugin did not respond correctly, and some additional debugging information (provided as a best effort) including the CPU architecture that the plugin was compiled for, the current CPU architecture, and the permissions of the plugin. Hopefully this will help users diagnose why their plugin is not loading.
We also added a
testdata/directory that is optionally populated with executables in various formats to help test the additional debugging information. The binaries are over 1 MB each though, so are not checked in, and the test will be skipped if they have not been compiled.The error message comes out looking like: