vault icon indicating copy to clipboard operation
vault copied to clipboard

Add plugin version to GRPC interface

Open swenson opened this issue 3 years ago • 3 comments

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
  • [x] Update identity/cubbyhole/sys/etc. special backends to support version method.
  • [x] Investigate failing cache tests
  • [ ] Forward version to backend for v5 db plugin
  • [ ] Add tests for db plugin
  • [x] Use more specific error for version method not present

swenson avatar Sep 09 '22 16:09 swenson

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{

tvoran avatar Sep 09 '22 22:09 tvoran

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

swenson avatar Sep 12 '22 18:09 swenson

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.

swenson avatar Sep 12 '22 22:09 swenson

@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 avatar Sep 14 '22 20:09 swenson

@tomhjp I think I addressed your concerns. PTAL.

swenson avatar Sep 15 '22 21:09 swenson

Thanks!

swenson avatar Sep 15 '22 23:09 swenson