[WIP] Runtime plugins #1861

jimil749 · 2021-07-02T10:33:08Z

Description

This is my first try to enable runtime plugin support in the Reva ecosystem. This PR introduces a new plugin package, which is responsible for loading the plugins using the hashicorp go-plugin library. The current implementation uses Remote Procedure Call (RPC) to communicate between the host(reva core) and the plugin. I tried to move the userprovider drivers to the runtime paradigm, which implies that a user load userprovider drivers (like json, ldap etc) at runtime by just pointing to the binary in the configuration.

Working

In order to facilitate backward compatibility, there aren't any major changes to the configuration. Also, loading plugins from the in-memory is also supported. If the user wishes to load external plugins, he/she needs to add an extra shared config:

[shared]
plugin = true

[grpc.services.userprovider]
driver = '/path/to/binary'

This will configure the services to load plugins from the plugins package rather than the registry. The plugins package creates an rpc client, loads the required plugin and returns an interface{} value, which is then asserted into required type by the respective services. It is then responsibility of the services to call the New method to initialize the plugin with the configurations.

Progress

Global Plugins flag: Option to load plugins from either registry or plugin package
Complementing the existing userprovider JSON driver with runtime paradigm.
Plugin Package
- Support for loading plugin binaries
- Support for compiling and loading the plugin from source
- Support for plugins hosted as a version controlled repository
  ~~- [ ] Watcher module (Hot Reload)~~
  ~~- [ ] Separate dev and prod environment~~

Since, there maybe a lot of changes involved with the approach I've taken, I am creating a draft PR. Any comments for improvement would be highly appreciated! :)

update-docs · 2021-07-02T10:33:11Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

examples/plugin/plugin.toml

internal/grpc/services/userprovider/userprovider.go

pkg/user/user.go

ishank011 · 2021-07-05T08:39:23Z

@jimil749 can you also push the changes in the json pkg? It'll help me review the code

ishank011 · 2021-07-05T08:44:51Z

@jimil749 can you also push the changes in the json pkg? It'll help me review the code

Or do we have different code for the plugin and the registry paths?

jimil749 · 2021-07-05T09:14:30Z

@ishank011 sure. The plugin code is slightly different than the existing json pkg. I'll add the source code in the example directory.

ishank011 · 2021-07-05T09:34:11Z

Really good progress!

One thought about managing the configuration part. Can we have an interface in the plugin pkg with one method Configure(m map[string]interface{}) error and the manager interfaces in the drivers will embed this? Their New methods will be sort of wrappers around this method. And we can get rid of the duplicate interfaces and assume that all managers will have the Configure method. If this works, we can have the same driver code for both paradigms.

Also, a question. The ManagerRPC would work for all drivers, right? If you have two plugins, json and demo, just changing the path in the configuration should work?

jimil749 · 2021-07-05T09:41:57Z

Also, a question. The ManagerRPC would work for all drivers, right? If you have two plugins, json and demo, just changing the path in the configuration should work?

Just to quickly answer this: Yes! This method works for all the "userprovider" plugins. I tested it out with demo plugin and it works!

jimil749 · 2021-07-05T09:45:00Z

One thought about managing the configuration part. Can we have an interface in the plugin pkg with one method Configure(m map[string]interface{}) error and the manager interfaces in the drivers will embed this? Their New methods will be sort of wrappers around this method. And we can get rid of the duplicate interfaces and assume that all managers will have the Configure method. If this works, we can have the same driver code for both paradigms.

That's a great suggestion! Let me try to implement this locally and see how it goes. This would really improve the implementation.

jimil749 · 2021-07-05T12:58:27Z

One thought about managing the configuration part. Can we have an interface in the plugin pkg with one method Configure(m map[string]interface{}) error and the manager interfaces in the drivers will embed this? Their New methods will be sort of wrappers around this method. And we can get rid of the duplicate interfaces and assume that all managers will have the Configure method. If this works, we can have the same driver code for both paradigms.

@ishank011, why can't we just add the method to the manager interface, instead of embedding? I think that should also work? Because, embedding will add dependency on the plugins package. (which would cause cyclic imports)

We could simply add Configure method to all the managers and have the New methods as wrappers, which would use the Configure method to configure the plugins. Does that seem fine?

ishank011 · 2021-07-05T14:28:55Z

The dependency on the individual packages isn't ideal. Can we have a registry similar to what we have for the individual packages to populate pluginMap? The reason why I want to have this interface in the plugin package is to maintain consistency across packages. If we want to add new things sometime in the future, that would be a lot easier as well

jimil749 · 2021-07-05T15:40:09Z

@ishank011, thanks for the clarification! Makes sense to me. 👍 Also +1 for the registry way of populating the pluginMap. Implementing the same.

internal/grpc/services/userprovider/userprovider.go

ishank011 · 2021-07-07T11:59:06Z

Majorly looks good. I have a few nitpicks but we can get to those later on. We can proceed to the next step I guess? Let's discuss the compilation part in the issue

jimil749 · 2021-07-07T12:12:42Z

Sure!

jimil749 · 2021-07-14T09:55:10Z

pkg/plugin/plugin.go

+// Load loads the plugin using the hashicorp go-plugin system
+func Load(driver string, pluginType string) (interface{}, error) {
+	bin := driver
+	if filepath.Ext(driver) == ".go" {


Is this okay? I am currently assuming that if the user wants to compile, he/he can provide path to the source code ending with .go. So whenever the config ends with .go, we compile!

This path should point to a package as it can have multiple go files.

Ah okay. So then we can stat the path, and check if that's a directory, we compile else we directly load.

jimil749 · 2021-07-14T09:57:59Z

pkg/plugin/plugin.go

+		return "", fmt.Errorf("could not find current directory: %v", err)
+	}
+	name := fmt.Sprintf("%d", rand.Int())
+	pluginsDir := filepath.Join(wd, "bin", pluginType, name)


I store the binaries in ./bin/userprovider/<rand_int>. Is this fine?

Why a random integer? Let's use ./bin/userprovider/driver_name, we can replace the old binaries and won't have to worry about cleaning those up.

Yeah makes sense. I'll make the changes

jimil749 · 2021-07-14T10:07:45Z

@ishank011, I'm a bit worried about one thing: with the current approach, loading a plugin binary works just fine. The hashicorp go-plugin spawns a new plugin process for us, which can be used over rpc. But we're not killing the process once we're done using it. So when I quit revad, the plugin process is not killed. We need to clean that up too.

jimil749 · 2021-07-14T10:12:11Z

internal/grpc/services/userprovider/userprovider.go

+	}
+
+	basename := filepath.Base(c.Driver)
+	pluginConfig := strings.TrimSuffix(basename, filepath.Ext(basename))


Again, I need to do this because, whenever a user provides configure like:

[grpc.services.userprovider] driver = /home/path/to/json.go [grpc.services.userprovider.json] users = users.demo.json

so I need to strip off, .go from the driver field to access to users field!

Pointing to the package should take care of that

ishank011 · 2021-07-14T10:17:21Z

The GRPC services have a Close method where we can call Kill if the plugin flag is set

pkg/plugin/plugin.go

jimil749 · 2021-07-22T11:44:10Z

@ishank011, currently this approach works just fine:

The plugin package is able to load plugin binary via the hashicorp go-plugin.
The plugin package is also able to first compile the source code, store the binary at ./bin/userprovider/<driver> and then load the binary. (Only constraint is that the user needs to provide relative path to the source code!). Package differentiates the binary file and source code using os.Stat. If the user points to a package/directory, we compile else we directly load the driver.
The older in-memory registry way loading is also supported, can be toggled by the plugin flag in the shared config.

Signed-off-by: Jimil Desai <[email protected]>

1. Global registry to populate the plugin map (plugin registry) 2. Plugin Config interface with `Configure(m map[string]interface{})` to configure plugins. Signed-off-by: Jimil Desai <[email protected]>

Signed-off-by: Jimil Desai <[email protected]>

jimil749 · 2021-08-03T15:48:47Z

pkg/user/rpc_user.go

+)
+
+func init() {
+	gob.Register(&userpb.User{})


We can register interfaces (which we would like to pass as context) here. But I am not sure what all structs/interfaces that we'd like to add!

Are we able to log from the plugins now?

I tried passing the user object as a part of the context. I was able to access the user object (via the context.Value) on the plugin side. But here's the problem with the logger: the appctx's WithLogger attaches zerolog's ctxKey struct to the context, which is not exported, hence we cannot register that at init time.

Can you look into ways of getting around that?

In the auth pkg, we're passing auth.Scope without registering it. How does that work?

I think this might be a roadblock as there is not way to gob.Register(ctxkey{}) at init stage because ctxKey{} is private! Hence the RPC is always going to fail when attaching logger to context.

Can you look into ways of getting around that?

Yes. I was just investigating ways around this!

I'll merge this for now but this needs to be addressed ASAP.

Yes! So the reason we're getting the gob error is because we're passing map[interface{}]interface{}, If we are passing interface{} through gob, that NEEDS to be registered, passing structs is fine. That's why it does not complain when using auth.Scope. https://pkg.go.dev/encoding/gob#Register I'll further dig into this

Then instead of the map, let's store all the keys that we know of in a struct. The ones we currently use are the logger, user object and the token.

Signed-off-by: Jimil Desai <[email protected]>

jimil749 · 2021-08-04T10:05:38Z

/cc @ishank011.

jimil749 commented Jul 2, 2021

View reviewed changes

examples/plugin/plugin.toml Outdated Show resolved Hide resolved

jimil749 commented Jul 2, 2021

View reviewed changes

internal/grpc/services/userprovider/userprovider.go Outdated Show resolved Hide resolved

jimil749 commented Jul 2, 2021

View reviewed changes

pkg/user/user.go Outdated Show resolved Hide resolved

jimil749 force-pushed the runtime-plugins branch from a03c165 to c2cefe4 Compare July 5, 2021 07:24

ishank011 requested a review from refs July 5, 2021 10:39

jimil749 linked an issue Jul 5, 2021 that may be closed by this pull request

Runtime Plugins: Design & integration #1814

Closed

jimil749 requested a review from ishank011 July 6, 2021 08:47

ishank011 reviewed Jul 7, 2021

View reviewed changes

internal/grpc/services/userprovider/userprovider.go Outdated Show resolved Hide resolved

jimil749 force-pushed the runtime-plugins branch from c3afa96 to a15b0b2 Compare July 7, 2021 11:51

ishank011 added the runtime-plugins label Jul 8, 2021

jimil749 commented Jul 14, 2021

View reviewed changes

ishank011 reviewed Jul 14, 2021

View reviewed changes

pkg/plugin/plugin.go Outdated Show resolved Hide resolved

jimil749 commented Jul 14, 2021

View reviewed changes

pkg/plugin/plugin.go Outdated Show resolved Hide resolved

jimil749 requested a review from ishank011 July 19, 2021 08:31

jimil749 added 24 commits August 3, 2021 21:12

changelog added

a84fb75

Signed-off-by: Jimil Desai <[email protected]>

added plugin source code

a3fa53e

Signed-off-by: Jimil Desai <[email protected]>

updated comments

26fd59e

Signed-off-by: Jimil Desai <[email protected]>

Introduced:

67300dd

1. Global registry to populate the plugin map (plugin registry) 2. Plugin Config interface with `Configure(m map[string]interface{})` to configure plugins. Signed-off-by: Jimil Desai <[email protected]>

add comments

a92ba05

Signed-off-by: Jimil Desai <[email protected]>

add more comments

98cb408

Signed-off-by: Jimil Desai <[email protected]>

fix: populate pluginregistry at init time

11c11cf

Signed-off-by: Jimil Desai <[email protected]>

resolve merge conflict

3f1234b

Signed-off-by: Jimil Desai <[email protected]>

feat: compilation of plugin source code

d1a590a

Signed-off-by: Jimil Desai <[email protected]>

fix: updated dirname and path

2c08741

Signed-off-by: Jimil Desai <[email protected]>

code refractor and client killing mechanism

51eb17e

Signed-off-by: Jimil Desai <[email protected]>

update driver path in example

cf42dec

Signed-off-by: Jimil Desai <[email protected]>

add comment

b91bb73

Signed-off-by: Jimil Desai <[email protected]>

code refractor

5b00d17

Signed-off-by: Jimil Desai <[email protected]>

updated compile binary location to /var/tmp/reva

9ed4373

Signed-off-by: Jimil Desai <[email protected]>

remove plugin flag check

ad41d49

Signed-off-by: Jimil Desai <[email protected]>

delegate the logic to choose b/w registry and plugin to plugin package

14cea90

Signed-off-by: Jimil Desai <[email protected]>

code refractor

32c6430

Signed-off-by: Jimil Desai <[email protected]>

update driver config

09e2859

Signed-off-by: Jimil Desai <[email protected]>

pass client context in rpc

084d5e1

Signed-off-by: Jimil Desai <[email protected]>

revert context passing and added support for auth_provider plugin

2990ddf

Signed-off-by: Jimil Desai <[email protected]>

update with appropriate comments

c8de61b

Signed-off-by: Jimil Desai <[email protected]>

remove extra white space

c06d057

Signed-off-by: Jimil Desai <[email protected]>

feat: use appctx methods to pass context via rpcs

ab793c3

Signed-off-by: Jimil Desai <[email protected]>

jimil749 force-pushed the runtime-plugins branch from dd53745 to ab793c3 Compare August 3, 2021 15:43

jimil749 commented Aug 3, 2021

View reviewed changes

return stderr from exec call as part of the error

c060977

Signed-off-by: Jimil Desai <[email protected]>

ishank011 approved these changes Aug 5, 2021

View reviewed changes

ishank011 merged commit 324f703 into cs3org:master Aug 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Runtime plugins #1861

[WIP] Runtime plugins #1861

jimil749 commented Jul 2, 2021 •

edited

Loading

update-docs bot commented Jul 2, 2021

ishank011 commented Jul 5, 2021

ishank011 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

ishank011 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

ishank011 commented Jul 5, 2021 •

edited

Loading

jimil749 commented Jul 5, 2021 •

edited

Loading

ishank011 commented Jul 7, 2021

jimil749 commented Jul 7, 2021

jimil749 Jul 14, 2021

ishank011 Jul 14, 2021

jimil749 Jul 14, 2021

jimil749 Jul 14, 2021

ishank011 Jul 14, 2021

jimil749 Jul 14, 2021

jimil749 commented Jul 14, 2021

jimil749 Jul 14, 2021

ishank011 Jul 14, 2021

ishank011 commented Jul 14, 2021

jimil749 commented Jul 22, 2021 •

edited

Loading

jimil749 Aug 3, 2021

ishank011 Aug 5, 2021

jimil749 Aug 5, 2021

ishank011 Aug 5, 2021

ishank011 Aug 5, 2021

jimil749 Aug 5, 2021

jimil749 Aug 5, 2021

ishank011 Aug 5, 2021

jimil749 Aug 5, 2021

ishank011 Aug 5, 2021

jimil749 commented Aug 4, 2021

[WIP] Runtime plugins #1861

[WIP] Runtime plugins #1861

Conversation

jimil749 commented Jul 2, 2021 • edited Loading

Description

Working

Progress

update-docs bot commented Jul 2, 2021

ishank011 commented Jul 5, 2021

ishank011 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

ishank011 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

jimil749 commented Jul 5, 2021

ishank011 commented Jul 5, 2021 • edited Loading

jimil749 commented Jul 5, 2021 • edited Loading

ishank011 commented Jul 7, 2021

jimil749 commented Jul 7, 2021

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

jimil749 commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishank011 commented Jul 14, 2021

jimil749 commented Jul 22, 2021 • edited Loading

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

jimil749 commented Aug 4, 2021

jimil749 commented Jul 2, 2021 •

edited

Loading

ishank011 commented Jul 5, 2021 •

edited

Loading

jimil749 commented Jul 5, 2021 •

edited

Loading

jimil749 commented Jul 22, 2021 •

edited

Loading