Add plugin version to GRPC interface #17088

swenson · 2022-09-09T16:54:36Z

Added a version interface in the sdk/logical so that it can be shared between all plugin types, and then wired it up to RunningVersion in the mounts, auth list, and database systems.

I've tested that this works with auth, database, and secrets plugin types, with the following logic to populate RunningVersion:

If a plugin has a Version() method implemented, then that is used
If not, and the plugin is built into the Vault binary, then the go.mod version is used
Otherwise, the it will be the empty string.

My apologies for the length of this PR.

tvoran · 2022-09-09T22:27:52Z

It looks like a lot of the failed tests are using the PassthroughBackend, so I wonder if that needs another type assertion in request_handling.go and the factory wrapped in request_handling_test.go:

diff --git a/vault/request_handling.go b/vault/request_handling.go
index b37085c83c..95243bb38a 100644
--- a/vault/request_handling.go
+++ b/vault/request_handling.go
@@ -1076,7 +1076,7 @@ func (c *Core) handleRequest(ctx context.Context, req *logical.Request) (retResp
                                return nil, auth, retErr
                        }
 
-                       if ptbe, ok := matchingBackend.(*PassthroughBackend); ok {
+                       if ptbe, ok := matchingBackend.(builtinVersionBackend).backend.(*PassthroughBackend); ok {
                                if !ptbe.GeneratesLeases() {
                                        registerLease = false
                                        resp.Secret.Renewable = false
diff --git a/vault/request_handling_test.go b/vault/request_handling_test.go
index cfdea525f7..b79601039b 100644
--- a/vault/request_handling_test.go
+++ b/vault/request_handling_test.go
@@ -17,7 +17,7 @@ import (
 func TestRequestHandling_Wrapping(t *testing.T) {
        core, _, root := TestCoreUnsealed(t)
 
-       core.logicalBackends["kv"] = PassthroughBackendFactory
+       core.logicalBackends["kv"] = wrapFactoryAddBuiltinVersion(PassthroughBackendFactory)
 
        meUUID, _ := uuid.GenerateUUID()
        err := core.mount(namespace.RootContext(nil), &MountEntry{

sdk/logical/logical.go

sdk/plugin/pb/backend.proto

sdk/plugin/grpc_backend_client.go

swenson · 2022-09-12T18:27:21Z

Thanks @tvoran for the tip about the passthroughbackend -- that was exactly the problem.

swenson · 2022-09-12T22:57:52Z

I converted this into a draft since I have a bit more work to do before it's quite ready. I went ahead and removed the plugin wrapper and used the build information to get the plugin version, as Tom suggested, which simplifies things a little.

Unlike the POC, I went ahead and added it into the main interface. So far in my testing, this has been safe to do. I also took a slightly different approach for how to include the `v1.12.0+builtin` version in the builtin plugins: I added a new wrapper struct that sets the version to the "builtin" version if it isn't already set, and I wrap all of the builtin factories with that when we create the map of factories. This way we don't have to update every builtin plugin to add the version logic. If we do want to version those plugins separately, though, we will be able to do so. There are still some TODOs before merging this, I feel, but this is getting large so I wanted to check in: * [ ] More manual testing with existing plugins and updated (with version) ones * [ ] Update identity/cubbyhole/sys/etc. special backends to support version method.

…since we won't need it anymore

We use a placeholder backend (previously a framework.Backend) before a GRPC plugin is lazy-loaded. This makes us later think the plugin is a builtin plugin. So we added a `placeholderBackend` type that overrides the `IsExternal()` method so that later we know that the plugin is external, and don't give it a default builtin version.

swenson · 2022-09-14T20:39:59Z

@tvoran @tomhjp this is ready for review, I believe. I think I addressed all of your comments, and I did a lot of testing.

swenson · 2022-09-14T20:42:58Z

vault/plugin_catalog.go

@@ -874,42 +869,3 @@ func isPluginType(s string) bool {
 	_, err := consts.ParsePluginType(s)
 	return err == nil
 }
-
-func (c *PluginCatalog) getBuiltinVersion(pluginType consts.PluginType, pluginName string) string {


I moved this function to more central place so it could be used more easily. It didn't really need access to the pluginCatalog to work.

tomhjp

Looking good! It might be that my comment about validating the self-reported version during registration would be best left for another PR if that's non-trivial

tomhjp · 2022-09-15T16:13:05Z

builtin/logical/database/path_config_connection.go

+		// don't override a manually specified plugin version
+		// but if it is empty, then add in the self-reported version for informational purposes
+		if config.PluginVersion == "" {
+			config.PluginVersion = dbw.Version().Version


I think this could cause issues the next time the config is updated if it's different to the version we did the initial lookup with, as next time we'll lookup the reported version and maybe get a different entry from the catalog. Could we call this method as we're registering the plugin, and either a) Use the reported version in the catalog if none was provided or b) Error if the reported version is different from the one provided? Then we shouldn't need to make any updates here as the catalog and reported versions will always match up.

It might be interesting to query the version from the plugin and validate that it's the version we think it should be or error out, though I can't think of any reason that would fail if we validate during plugin registration.

It is definitely possible to add a check between PluginVersion and the self-reported version on registration.

I primarily added this because, if no version is specified on registration, it is otherwise impossible to tell what version of the plugin is running, and I wanted to expose this somewhere.

It doesn't even really make sense to have this information in the config at all, but there's no mount or auth table for database configs.

tomhjp · 2022-09-15T16:23:24Z

builtin/plugin/backend.go

-	b.Backend = &framework.Backend{
-		PathsSpecial: paths,
-		BackendType:  btype,
+	b.Backend = &placeholderBackend{


placeholderBackend.IsExternal() always returns true, but the function comment for Backend implies the backend could be builtin. Not sure if I'm just misreading though?

Ah, you're right, the placeholder is used even for builtin plugins. I'll see if I can just forward the IsExternal method.

changelog/17088.txt

tomhjp · 2022-09-15T16:43:14Z

sdk/logical/version.proto

+}
+
+// Versioned is an optional RPC service implemented by plugins.
+service Versioned {


WDYT about PluginVersion to differentiate from Vault version and protocol version?

tomhjp · 2022-09-15T16:44:42Z

sdk/logical/version.proto

+// VersionReply is the reply for the Version method.
+message VersionReply {
+  string plugin_name = 1;
+  string version = 2;


I originally wanted to use version everywhere, so started sprinkling that around. But then we've found a couple of places where we need to use plugin_version to avoid ambiguity. I'm wondering if we should standardise on plugin_version everywhere for consistency, and for an abundance of clarity?

I think that makes sense. I'll rename it.

tomhjp · 2022-09-15T16:52:43Z

sdk/plugin/grpc_backend_client.go

+				return logical.EmptyVersion
+			}
+		}
+		b.Logger().Warn("Unknown error getting plugin version", "err", err)


Something would have to go seriously wrong for this to happen, which makes me a bit nervous to only log it server-side and continue. I see Type does something similar above, but perhaps it's worth adding an error return value to our new interface signature before we first ship it so that we have an easier time if we have need for it at a later date. WDYT?

I was trying to mimic Type's relative safety -- worst case scenario, the Version will just return "". I can think of few reasons why this might fail, like if the plugin dies or is running too slowly.

(Type won't even log a warning if an error occurs. :))

I'm open to adding an error return though; I was just trying to keep it as simple as possible.

Yeah it's definitely nice to have that simplicity. I was thinking a little of possible future changes to the plugin architecture, e.g. what if we wanted to support remote plugins where transport errors might become less unusual. It's perfectly possible the error will never be a particularly useful part of the API though, so I'm not 100% about it.

sdk/plugin/grpc_backend_server.go

vault/auth.go

* Change changelog to improvement * Make IsExternal configurable for plugin placeholder * Start changing version to pluginVersion everywhere

swenson · 2022-09-15T21:12:15Z

@tomhjp I think I addressed your concerns. PTAL.

tomhjp

LGTM!

tvoran

Just a couple naive questions.

builtin/plugin/backend.go

sdk/framework/backend.go

swenson · 2022-09-15T23:36:25Z

Thanks!

swenson requested review from tvoran and tomhjp September 9, 2022 16:54

Base automatically changed from run-versioned-plugins to main September 9, 2022 17:14

swenson force-pushed the vault-8143/plugin-version-grpc branch from d590e4b to 0886a5d Compare September 9, 2022 17:17

tomhjp reviewed Sep 12, 2022

View reviewed changes

sdk/logical/logical.go Outdated Show resolved Hide resolved

sdk/plugin/pb/backend.proto Outdated Show resolved Hide resolved

sdk/plugin/pb/backend.proto Outdated Show resolved Hide resolved

sdk/plugin/grpc_backend_client.go Outdated Show resolved Hide resolved

swenson requested a review from a team September 12, 2022 22:55

swenson marked this pull request as draft September 12, 2022 22:56

Christopher Swenson added 7 commits September 13, 2022 10:44

Add plugin/running version to identity, cubbyhole, kv, sys mounts

cde1222

Fix cache tests; thanks to tvoran

1a5ce8a

Starting to work on addressing feedback; removing the plugin wrapper …

f6069d4

…since we won't need it anymore

Despite what the comment said, plugin.backend also wraps builtin plugins

64f4876

Fix two more mount tests

46eb9e5

Add changelog

4626aa8

swenson force-pushed the vault-8143/plugin-version-grpc branch from 34ec9ba to 4626aa8 Compare September 13, 2022 17:47

Move version out of database client

fdf2894

swenson force-pushed the vault-8143/plugin-version-grpc branch from 6e99f04 to fdf2894 Compare September 13, 2022 20:04

Christopher Swenson added 9 commits September 13, 2022 13:11

Remove unnecessary changes from PR

362722c

Remove more unnecessary changes from PR

55fedd1

Add versioned client to test for expected response

72d6593

Register the grpc servers

fb69d24

Forward version

7b34858

Add database version tests

b74a6e8

make fmt

a84f6e4

Plumb the database version all the way through to the plugin

17e1d44

swenson marked this pull request as ready for review September 14, 2022 20:38

Make fmt

b83d42b

swenson commented Sep 14, 2022

View reviewed changes

tomhjp added this to the 1.12 milestone Sep 15, 2022

Don't override non-empty database PluginVersion

f81db36

tomhjp reviewed Sep 15, 2022

View reviewed changes

Address PR feedback part 1

377d8ee

* Change changelog to improvement * Make IsExternal configurable for plugin placeholder * Start changing version to pluginVersion everywhere

swenson modified the milestones: 1.12, 1.12.0-rc1 Sep 15, 2022

Rename version to plugin version

0e6d9e6

swenson force-pushed the vault-8143/plugin-version-grpc branch from 7e7581a to 0e6d9e6 Compare September 15, 2022 20:36

tomhjp approved these changes Sep 15, 2022

View reviewed changes

tvoran reviewed Sep 15, 2022

View reviewed changes

builtin/plugin/backend.go Show resolved Hide resolved

sdk/framework/backend.go Show resolved Hide resolved

tvoran approved these changes Sep 15, 2022

View reviewed changes

swenson merged commit 70278c2 into main Sep 15, 2022

swenson deleted the vault-8143/plugin-version-grpc branch September 15, 2022 23:38

tomhjp mentioned this pull request Sep 27, 2022

Plugin versioning changelog entries #17322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plugin version to GRPC interface #17088

Add plugin version to GRPC interface #17088

swenson commented Sep 9, 2022 •

edited

Loading

tvoran commented Sep 9, 2022 •

edited

Loading

swenson commented Sep 12, 2022

swenson commented Sep 12, 2022

swenson commented Sep 14, 2022

swenson Sep 14, 2022

tomhjp left a comment

tomhjp Sep 15, 2022

swenson Sep 15, 2022

tomhjp Sep 15, 2022

swenson Sep 15, 2022

tomhjp Sep 15, 2022

swenson Sep 15, 2022

tomhjp Sep 15, 2022

swenson Sep 15, 2022

tomhjp Sep 15, 2022

swenson Sep 15, 2022

tomhjp Sep 15, 2022

swenson commented Sep 15, 2022

tomhjp left a comment

tvoran left a comment

swenson commented Sep 15, 2022

Add plugin version to GRPC interface #17088

Add plugin version to GRPC interface #17088

Conversation

swenson commented Sep 9, 2022 • edited Loading

tvoran commented Sep 9, 2022 • edited Loading

swenson commented Sep 12, 2022

swenson commented Sep 12, 2022

swenson commented Sep 14, 2022

Choose a reason for hiding this comment

tomhjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swenson commented Sep 15, 2022

tomhjp left a comment

Choose a reason for hiding this comment

tvoran left a comment

Choose a reason for hiding this comment

swenson commented Sep 15, 2022

swenson commented Sep 9, 2022 •

edited

Loading

tvoran commented Sep 9, 2022 •

edited

Loading