Skip to content
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

Support running plugins in isolated containers #22712

Merged
merged 15 commits into from Sep 1, 2023

Conversation

tomhjp
Copy link
Collaborator

@tomhjp tomhjp commented Aug 31, 2023

This implements running plugins in containers to give them some degree of isolation from the main Vault process and other plugins. It only supports running on Linux initially, where it is easiest to manage unix socket communication across the container boundary.

The plugin lifecycle and multiplexing etc remains managed by go-plugin and therefore unchanged. See hashicorp/go-plugin#270 for the main related go-plugin PR, and hashicorp/go-secure-stdlib#84 for the bulk of the go-secure-stdlib piece.

The bulk of the implementation for running plugins in a container is in the new github.com/hashicorp/go-secure-stdlib/plugincontainer module, which depends on go-plugin's v1.5.0 updates to make the command execution pluggable.

The main items done for this PR are:

  • Support a new oci_image option when registering a plugin
  • Thread that through into the plugin catalog
  • Configure the plugincontainer library appropriately when we run an image from the catalog with oci_image specified
  • Some basic integration tests to ensure the end to end flow works

Follow ups will include:

  • Linking up with the output of add plugin runtime API #22469 to support configuring more attributes of the plugin's container
  • Make gVisor's runsc runtime the default (will require installing it on the CI runners)
  • Make the plugin_directory config setting optional as long as users are only registering containerised plugins
  • Finalise any labels we should set on running containers
  • API and usage documentation

@tomhjp tomhjp added this to the 1.15 milestone Aug 31, 2023
@tomhjp tomhjp requested a review from a team as a code owner August 31, 2023 22:23
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Aug 31, 2023
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@github-actions
Copy link

github-actions bot commented Aug 31, 2023

CI Results:
All Go tests succeeded! ✅

Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested, and this works great locally!

Just a small comment, and a suggestion that helped me with testing, at least.

"managed-by": "hashicorp.com/vault",
},
// TODO: More configurables.
// Defaulting to runsc will require installing gVisor in the GitHub runner.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will probably want to check that the runsc binary is available and executable before defaulting to it, too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to note my plans here in the description, but that's a good shout. What I'll probably do is error if runsc isn't available in $PATH, and if people want to use runc for example, they'll need to create their own Vault "plugin runtime" in the Vault API and reference it when registering the plugin.

That lets us use a safe default that behaves predictably, and hopefully still with a helpful enough UX, because I wouldn't want Vault to suddenly start using a different container runtime if runsc gets uninstalled out of band.

vault/testdata/Dockerfile Outdated Show resolved Hide resolved
command/plugin_register.go Show resolved Hide resolved
Also adds a test for checking the JSON body generated by the command, and
corrects the help text for -args which suggested a form that doesn't work.

Finally, updates the plugin testing Dockerfile image to use ubuntu as a base
so it's more forgiving for any additional testing run on top of it
Historically it's been omitted, and it could conceivably have secret information in
it, so if we want to return it in the response, it should probably only be via explicit
opt-in. Skipping for now though as it's not the main purpose of the PR
Copy link
Contributor

@swenson swenson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work!

api/sys_plugins.go Show resolved Hide resolved
vault/logical_system.go Outdated Show resolved Hide resolved
@tomhjp tomhjp enabled auto-merge (squash) September 1, 2023 17:41
@tomhjp tomhjp merged commit 07e7619 into main Sep 1, 2023
98 checks passed
@tomhjp tomhjp deleted the vault-18182/plugin-container-execution branch September 1, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants