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

plugins: add support for plugin configs #6613

Merged
merged 1 commit into from
Aug 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 2 additions & 30 deletions cmd/ipfs/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"fmt"
"math/rand"
"os"
"path/filepath"
"runtime/pprof"
"strings"
"time"
Expand Down Expand Up @@ -46,22 +45,9 @@ const (
)

func loadPlugins(repoPath string) (*loader.PluginLoader, error) {
pluginpath := filepath.Join(repoPath, "plugins")

plugins, err := loader.NewPluginLoader()
plugins, err := loader.NewPluginLoader(repoPath)
Copy link
Member Author

Choose a reason for hiding this comment

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

I needed the repo path so I figured I might as well move a bunch of the plugin loading logic into the loader.

if err != nil {
return nil, fmt.Errorf("error loading preloaded plugins: %s", err)
}

// check if repo is accessible before loading plugins
ok, err := checkPermissions(repoPath)
Copy link
Member Author

Choose a reason for hiding this comment

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

This was kind of useless so I didn't bother replicating it. Instead of checking permissions, we just do the thing and see if it fails.

if err != nil {
return nil, err
}
if ok {
if err := plugins.LoadDirectory(pluginpath); err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

Called in NewPluginLoader

return nil, err
}
return nil, fmt.Errorf("error loading plugins: %s", err)
}

if err := plugins.Initialize(); err != nil {
Expand Down Expand Up @@ -282,20 +268,6 @@ func makeExecutor(req *cmds.Request, env interface{}) (cmds.Executor, error) {
return http.NewClient(host, opts...), nil
}

func checkPermissions(path string) (bool, error) {
_, err := os.Open(path)
if os.IsNotExist(err) {
// repo does not exist yet - don't load plugins, but also don't fail
return false, nil
}
if os.IsPermission(err) {
// repo is not accessible. error out.
return false, fmt.Errorf("error opening repository at %s: permission denied", path)
}

return true, nil
}

// commandDetails returns a command's details for the command given by |path|.
func commandDetails(path []string) cmdDetails {
if len(path) == 0 {
Expand Down
6 changes: 6 additions & 0 deletions commands/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ func (c *Context) GetNode() (*core.IpfsNode, error) {
return nil, errors.New("nil ConstructNode function")
}
c.node, err = c.ConstructNode()
if err == nil {
// Pre-load the config from the repo to avoid re-parsing it from disk.
if cfg, err := c.node.Repo.Config(); err != nil {
c.config = cfg
}
}
}
return c.node, err
}
Expand Down
3 changes: 3 additions & 0 deletions docs/plugins.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ directory (by default `~/.ipfs/plugins`).

## Plugin Types

Plugins can implement one or more plugin types, defined in the
[plugin](https://godoc.org/github.com/ipfs/go-ipfs/plugin) package.

### IPLD

IPLD plugins add support for additional formats to `ipfs dag` and other IPLD
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ require (
github.com/ipfs/go-ipfs-blockstore v0.1.0
github.com/ipfs/go-ipfs-chunker v0.0.1
github.com/ipfs/go-ipfs-cmds v0.1.0
github.com/ipfs/go-ipfs-config v0.0.6
github.com/ipfs/go-ipfs-config v0.0.11
github.com/ipfs/go-ipfs-ds-help v0.0.1
github.com/ipfs/go-ipfs-exchange-interface v0.0.1
github.com/ipfs/go-ipfs-exchange-offline v0.0.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@ github.com/ipfs/go-ipfs-chunker v0.0.1/go.mod h1:tWewYK0we3+rMbOh7pPFGDyypCtvGcB
github.com/ipfs/go-ipfs-cmds v0.1.0 h1:0CEde9EcxByej8+L6d1PST57J4ambRPyCTjLG5Ymou8=
github.com/ipfs/go-ipfs-cmds v0.1.0/go.mod h1:TiK4e7/V31tuEb8YWDF8lN3qrnDH+BS7ZqWIeYJlAs8=
github.com/ipfs/go-ipfs-config v0.0.5/go.mod h1:IGkVTacurWv9WFKc7IBPjHGM/7hi6+PEClqUb/l2BIM=
github.com/ipfs/go-ipfs-config v0.0.6 h1:jzK9Tl8S0oWBir3F5ObtGgnHRPdqQ0MYiCmwXtV3Ps4=
github.com/ipfs/go-ipfs-config v0.0.6/go.mod h1:IGkVTacurWv9WFKc7IBPjHGM/7hi6+PEClqUb/l2BIM=
github.com/ipfs/go-ipfs-config v0.0.11 h1:5/4nas2CQXiKr2/MLxU24GDGTBvtstQIQezuk7ltOQQ=
github.com/ipfs/go-ipfs-config v0.0.11/go.mod h1:wveA8UT5ywN26oKStByzmz1CO6cXwLKKM6Jn/Hfw08I=
github.com/ipfs/go-ipfs-delay v0.0.0-20181109222059-70721b86a9a8/go.mod h1:8SP1YXK1M1kXuc4KJZINY3TQQ03J2rwBG9QfXmbRPrw=
github.com/ipfs/go-ipfs-delay v0.0.1 h1:r/UXYyRcddO6thwOnhiznIAiSvxMECGgtv35Xs1IeRQ=
github.com/ipfs/go-ipfs-delay v0.0.1/go.mod h1:8SP1YXK1M1kXuc4KJZINY3TQQ03J2rwBG9QfXmbRPrw=
Expand Down
34 changes: 30 additions & 4 deletions plugin/loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ package loader
import (
"fmt"
"os"
"path/filepath"
"strings"

config "github.com/ipfs/go-ipfs-config"
cserialize "github.com/ipfs/go-ipfs-config/serialize"
coredag "github.com/ipfs/go-ipfs/core/coredag"
plugin "github.com/ipfs/go-ipfs/plugin"
fsrepo "github.com/ipfs/go-ipfs/repo/fsrepo"
Expand Down Expand Up @@ -83,16 +86,32 @@ type PluginLoader struct {
state loaderState
plugins map[string]plugin.Plugin
started []plugin.Plugin
config config.Plugins
repo string
}

// NewPluginLoader creates new plugin loader
func NewPluginLoader() (*PluginLoader, error) {
loader := &PluginLoader{plugins: make(map[string]plugin.Plugin, len(preloadPlugins))}
func NewPluginLoader(repo string) (*PluginLoader, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The repo a Plugin receives from the Environment should probably be normalized into absolute paths.
Not sure though, there may be a case for accepting relatives.

Abs patch 1/2

Suggested change
func NewPluginLoader(repo string) (*PluginLoader, error) {
func NewPluginLoader(repo string) (*PluginLoader, error) {
var err error
if repo, err = filepath.Abs(repo); err != nil {
return nil, err
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Given that we don't do this anywhere else, I'd like to leave this as it is for consistency (that way the repo path is the same everywhere). I believe it'll usually be an absolute path anyways.

If we do run into issues, we should be consistent and make this path absolute at a higher layer.

loader := &PluginLoader{plugins: make(map[string]plugin.Plugin, len(preloadPlugins)), repo: repo}
if repo != "" {
cfg, err := cserialize.Load(filepath.Join(repo, config.DefaultConfigFile))
switch err {
case cserialize.ErrNotInitialized:
case nil:
loader.config = cfg.Plugins
default:
return nil, err
}
}
for _, v := range preloadPlugins {
if err := loader.Load(v); err != nil {
return nil, err
}
}

if err := loader.LoadDirectory(filepath.Join(repo, "plugins")); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Abs patch 2/2

Suggested change
if err := loader.LoadDirectory(filepath.Join(repo, "plugins")); err != nil {
if err = loader.LoadDirectory(filepath.Join(repo, "plugins")); err != nil {

Copy link
Member Author

Choose a reason for hiding this comment

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

In general, I don't like using = in assignments inside if statements. The point of using an if statement like this is to scope the err variable to the if statement. Using a normal assignment lets it escape the scope and can be really confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, this should either be left as-is or should be:

err = loader.LoadDirectory(...)
if err != nil {
  // ...
}

return nil, err
}
return loader, nil
}

Expand Down Expand Up @@ -125,6 +144,10 @@ func (loader *PluginLoader) Load(pl plugin.Plugin) error {
"while trying to load dynamically: %s",
name, ppl.Version(), pl.Version())
}
if loader.config.Plugins[name].Disabled {
log.Infof("not loading disabled plugin %s", name)
return nil
}
loader.plugins[name] = pl
return nil
}
Expand Down Expand Up @@ -164,8 +187,11 @@ func (loader *PluginLoader) Initialize() error {
if err := loader.transition(loaderLoading, loaderInitializing); err != nil {
return err
}
for _, p := range loader.plugins {
err := p.Init()
for name, p := range loader.plugins {
err := p.Init(&plugin.Environment{
Repo: loader.repo,
Config: loader.config.Plugins[name].Config,
})
if err != nil {
loader.state = loaderFailed
return err
Expand Down
15 changes: 14 additions & 1 deletion plugin/plugin.go
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
package plugin

// Environment is the environment passed into the plugin on init.
type Environment struct {
// Path to the IPFS repo.
Repo string

// The plugin's config, if specified.
Config interface{}
}

// Plugin is base interface for all kinds of go-ipfs plugins
// It will be included in interfaces of different Plugins
type Plugin interface {
// Name should return unique name of the plugin
Name() string

// Version returns current version of the plugin
Version() string

// Init is called once when the Plugin is being loaded
Init() error
// The plugin is passed an environment containing the path to the
// (possibly uninitialized) IPFS repo and the plugin's config.
Init(env *Environment) error
}
2 changes: 1 addition & 1 deletion plugin/plugins/badgerds/badgerds.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func (*badgerdsPlugin) Version() string {
return "0.1.0"
}

func (*badgerdsPlugin) Init() error {
func (*badgerdsPlugin) Init(_ *plugin.Environment) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion plugin/plugins/flatfs/flatfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (*flatfsPlugin) Version() string {
return "0.1.0"
}

func (*flatfsPlugin) Init() error {
func (*flatfsPlugin) Init(_ *plugin.Environment) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion plugin/plugins/git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func (*gitPlugin) Version() string {
return "0.0.1"
}

func (*gitPlugin) Init() error {
func (*gitPlugin) Init(_ *plugin.Environment) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion plugin/plugins/levelds/levelds.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ func (*leveldsPlugin) Version() string {
return "0.1.0"
}

func (*leveldsPlugin) Init() error {
func (*leveldsPlugin) Init(_ *plugin.Environment) error {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion repo/fsrepo/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ var measureConfig = []byte(`{
}`)

func TestDefaultDatastoreConfig(t *testing.T) {
loader, err := loader.NewPluginLoader()
loader, err := loader.NewPluginLoader("")
if err != nil {
t.Fatal(err)
}
Expand Down
4 changes: 2 additions & 2 deletions test/sharness/t0020-init.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ test_expect_success "ipfs init fails" '
# Under Windows/Cygwin the error message is different,
# so we use the STD_ERR_MSG prereq.
if test_have_prereq STD_ERR_MSG; then
init_err_msg="Error: error opening repository at $IPFS_PATH: permission denied"
init_err_msg="Error: error loading plugins: open $IPFS_PATH/config: permission denied"
else
init_err_msg="Error: mkdir $IPFS_PATH: The system cannot find the path specified."
init_err_msg="Error: error loading plugins: open $IPFS_PATH/config: The system cannot find the path specified."
fi

test_expect_success "ipfs init output looks good" '
Expand Down
30 changes: 30 additions & 0 deletions test/sharness/t0280-plugin-data/example.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package main

import (
"fmt"
"os"

"github.com/ipfs/go-ipfs/plugin"
)

var Plugins = []plugin.Plugin{
&testPlugin{},
}

var _ = Plugins // used

type testPlugin struct{}

func (*testPlugin) Name() string {
return "test-plugin"
}

func (*testPlugin) Version() string {
return "0.1.0"
}

func (*testPlugin) Init(env *plugin.Environment) error {
fmt.Fprintf(os.Stderr, "testplugin %s\n", env.Repo)
fmt.Fprintf(os.Stderr, "testplugin %v\n", env.Config)
return nil
}
61 changes: 61 additions & 0 deletions test/sharness/t0280-plugin.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@ test_description="Test plugin loading"

. lib/test-lib.sh

if ! test_have_prereq PLUGIN; then
skip_all='skipping plugin tests, plugins not available'

test_done
fi

test_init_ipfs

test_expect_success "ipfs id succeeds" '
Expand All @@ -28,4 +34,59 @@ test_expect_success "cleanup bad plugin" '
rm "$IPFS_PATH/plugins/foo.so"
'

test_expect_success "install test plugin" '
go build \
-asmflags=all="-trimpath=${GOPATH}" -gcflags=all="-trimpath=${GOPATH}" \
-buildmode=plugin -o "$IPFS_PATH/plugins/example.so" ../t0280-plugin-data/example.go &&
chmod +x "$IPFS_PATH/plugins/example.so"
'

test_plugin() {
local loads="$1"
local repo="$2"
local config="$3"

rm -f id_raw_output id_output id_output_expected

test_expect_success "id runs" '
ipfs id 2>id_raw_output >/dev/null
'

test_expect_success "filter test plugin output" '
sed -ne "s/^testplugin //p" id_raw_output >id_output
'

if [ "$loads" != "true" ]; then
test_expect_success "plugin doesn't load" '
test_must_be_empty id_output
'
else
test_expect_success "plugin produces the correct output" '
echo "$repo" >id_output_expected &&
echo "$config" >>id_output_expected &&
test_cmp id_output id_output_expected
'
fi
}

test_plugin true "$IPFS_PATH" "<nil>"

test_expect_success "disable the plugin" '
ipfs config --json Plugins.Plugins.test-plugin.Disabled true
'

test_plugin false

test_expect_success "re-enable the plugin" '
ipfs config --json Plugins.Plugins.test-plugin.Disabled false
'

test_plugin true "$IPFS_PATH" "<nil>"

test_expect_success "configure the plugin" '
ipfs config Plugins.Plugins.test-plugin.Config foobar
'

test_plugin true "$IPFS_PATH" "foobar"

test_done