Multi plugins, plugin .conf filles #598
Conversation
so github.com/aryszka/configfilter could be converted to a plugin
Conflicts: cmd/skipper/main.go
* when loading a .so file, it is checked if there is also a .conf file with the same base, e.g. for `./plugins/noop.so` a config of `./plugins/noop.conf` is attempted to open. If it exists, each line in this file is passed as argument to the plugin loader function (leading and trailing spaces stripped, lines beginning with a `#` after space stripping are ignored). * add support for multitype plugins (this time with complete auto detection and `.conf` file support) * add some noop plugins in _test_plugins and add a make target to build and try loading as test
|
looking at https://github.com/zalando-stups/skrop I wonder if we should now change the signature of the ...similar for the |
|
@vetinari about It would be nice, but how would the config arguments be assigned to the different filters? |
plugins.go
Outdated
| @@ -116,16 +107,98 @@ func findAndLoadPlugins(o *Options) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (o *Options) LoadMultiPlugins(found map[string]string) error { | |||
There was a problem hiding this comment.
I think that the methods matching Load.+Plugins? here should not be exported. Is there a reason for exporting them, or a use case for applications that import skipper? I see that they are already exported in the master, but maybe it's not late to change this, if there is no reasonable use case for it. What may it break if unexporting them all?
plugins.go
Outdated
| } | ||
|
|
||
| func loadMultiPlugin(sym plugin.Symbol, path string, args []string) (filters.Spec, routing.PredicateSpec, routing.DataClient, error) { | ||
| fn, ok := sym.(func([]string) (filters.Spec, routing.PredicateSpec, routing.DataClient, error)) |
There was a problem hiding this comment.
How would it be to have an InitPlugin(opts []string) ([]filters.Spec, []routing.Predicate, []routing.DataClient) generic method with slices of return values, that can return multiple of any filters, predicates and data clients? This could be the generic plugin interface, while we could still keep the InitFilter, InitPredicate and InitDataClient interfaces to support the implementations of simpler plugins, with a single return value.
plugins.go
Outdated
| return fmt.Errorf("failed to read config for %s: %s", path, err) | ||
| } | ||
|
|
||
| if sym, err := mod.Lookup("InitPlugin"); err == nil { |
There was a problem hiding this comment.
Since we give multiple passes for loading the plugins, and we drop the "found" file entries in the first pass, it may happen that a plugin implements both InitPlugin and InitFilter, and only the InitPlugin will be evaluated, not the InitFilter. To avoid any confusion, I would simply consider it an error when a plugin implements more than one of: InitPlugin, InitFilter, InitPredicate, InitDataClient.
plugins.go
Outdated
| if dc != nil { | ||
| o.CustomDataClients = append(o.CustomDataClients, dc) | ||
| } | ||
| fmt.Printf("mutlitype plugin %s loaded from %s (filter: %t, predicate: %t, dataclient: %t)\n", |
There was a problem hiding this comment.
We follow the convention of printing log messages to the stderr. Please, change 'fmt.Println' to log.Println, using logrus. (Logrus allows the user to override the output.)
skipper.go
Outdated
| @@ -368,6 +368,10 @@ type Options struct { | |||
| // what the []string should contain. | |||
| DataClientPlugins [][]string | |||
|
|
|||
| // MultiPlugins combine multiple types of the above plugin types in one plugin (where | |||
| // necessary because of shared data between e.g. a filter and a data client). | |||
| MultiPlugins [][]string | |||
There was a problem hiding this comment.
If we consider the multi plugin the generic form of plugins, we could also just call this option simply: Plugins. What do you think?
|
ok, see my other comment. What I suggest basically, there should be a possibility to define a generic |
InitPlugin is now ``` InitPlugin(opts []string) ([]filters.Spec, []routing.Predicate, []routing.DataClient, error) ``` also: * any plugin which implements multiple Init* causes an error * use logrus
|
👍 |
1 similar comment
|
👍 |
so https://github.com/aryszka/configfilter could be converted to a plugin ;-)
.sofile, it is checked if there is also a.conffilewith the same base, e.g. for
./plugins/noop.soa config of./plugins/noop.confis attempted to open. If it exists, each linein this file is passed as argument to the plugin loader function
(leading and trailing spaces stripped, lines beginning with a
#after space stripping are ignored).
.conffile support
and try loading as test