-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Avoid Vault hang when no communication established with plugin #22914
Conversation
Also fixes a function where we may call go-plugin's client.Client() without ever calling client.Kill(), which could leak plugin processes
Build Results: |
CI Results: |
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.
When testing this manually with the bad docker config, I did notice that plugin register
returns success, even though it's erroring out on the vault side, not sure if that's expected behavior. But on the whole it's much more robust now 👍.
Thanks for giving it a spin!
Were the errors in log messages? I think we should probably find a way to clean those up a bit because they're often a bit too noisy, but yeah that's expected behaviour. While the plugin catalog is trying to get metadata from the plugins (during registration) the errors are just treated as warnings/debug info, but you should find that you get an error when you try to mount the plugin. |
Thanks! |
@tomhjp Gotcha, that makes sense. And yeah, most of the logs were
And mounting it does fail:
|
Also fixes a function where we may call go-plugin's client.Client() without ever calling client.Kill(), which could leak plugin processes.
It's pretty tricky to write an automated test for this, I'm still working on it, but wanted to get the change in before code freeze. To test manually, install runsc, and then break it for plugins by removing the additional runsc args we require - i.e. /etc/docker/daemon.json should look like this:
Then run
sudo systemctl reload docker
and run some of the tests inexternal_plugin_container_test.go
. Prior to this change it will hang indefinitely and then leave docker containers hanging around if you kill it. After this change, it should fail quickly and clean up after itself.