Enforce unique plugin names

 - Plugins names must be globally unique
 - Better handling of deprecated config in namespace manager
 - Updated config descriptions

Signed-off-by: Alex Shorsher <alex.shorsher@kaleido.io>

docs/reference/config.md

@@ -617,6 +617,7 @@ nav_order: 3
 |Key|Description|Type|Default Value|
 |---|-----------|----|-------------|
 |description|A description for the namespace|`string`|`<nil>`
+|mode|The namespace mode|`string`|`<nil>`
 |name|The name of the namespace (must be unique)|`string`|`<nil>`
 |plugins|The list of plugins for this namespace|`string`|`<nil>`
 |remoteName|The namespace name to be sent in plugin calls, if it differs from namespace name|`string`|`<nil>`

internal/coremsgs/en_config_descriptions.go

@@ -254,7 +254,7 @@ var (
 	ConfigNamespacesPredefined            = ffc("config.namespaces.predefined", "A list of namespaces to ensure exists, without requiring a broadcast from the network", "List "+i18n.StringType)
 	ConfigNamespacesPredefinedName        = ffc("config.namespaces.predefined[].name", "The name of the namespace (must be unique)", i18n.StringType)
 	ConfigNamespacesPredefinedPlugins     = ffc("config.namespaces.predefined[].plugins", "The list of plugins for this namespace", i18n.StringType)
-	ConfigNamespacesPredefinedMode        = ffc("config.namespaces.predefined[].mode", "The namespace mode", i18n.StringType)
+	ConfigNamespacesPredefinedMode        = ffc("config.namespaces.predefined[].mode", "The namespace mode. Valid values: gateway, multiparty", i18n.StringType)
 	ConfigNamespacesPredefinedRemoteName  = ffc("config.namespaces.predefined[].remoteName", "The namespace name to be sent in plugin calls, if it differs from namespace name", i18n.StringType)
 	ConfigNamespacesPredefinedDescription = ffc("config.namespaces.predefined[].description", "A description for the namespace", i18n.StringType)
 

internal/coremsgs/en_error_messages.go

@@ -234,11 +234,12 @@ var (
 	MsgInvalidOutputOption                = ffe("FF10385", "invalid output option '%s'")
 	MsgInvalidPluginConfiguration         = ffe("FF10386", "Invalid %s plugin configuration - name and type are required")
 	MsgReferenceMarkdownMissing           = ffe("FF10387", "Reference markdown file missing: '%s'")
-	MsgFFSystemReservedName               = ffe("FF10388", "Invalid namespace configuration - ff_system is a reserved name")
+	MsgFFSystemReservedName               = ffe("FF10388", "Invalid namespace configuration - %s is a reserved name")
 	MsgInvalidNamespaceMode               = ffe("FF10389", "Invalid %s namespace configuration - unknown mode")
 	MsgNamespaceUnknownPlugin             = ffe("FF10390", "Invalid %s namespace configuration - unknown plugin %s")
 	MsgNamespaceMultipartyConfiguration   = ffe("FF10391", "Invalid %s multiparty namespace configuration - database, blockchain, shared storage, and data exchange plugins are required")
 	MsgNamespaceGatewayNoDB               = ffe("FF10392", "Invalid %s gateway namespace configuration - a database plugin is required")
 	MsgNamespaceGatewayInvalidPlugins     = ffe("FF10393", "Invalid %s gateway namespace configuration - cannot specify dataexchange or shared storage plugins")
 	MsgNamespaceGatewayMultiplePluginType = ffe("FF10394", "Invalid %s namespace configuration - multiple %s plugins provided")
+	MsgDuplicatePluginName                = ffe("FF10395", "Invalid plugin configuration - plugin with name %s already exists")
 )

internal/namespace/config.go

@@ -36,10 +36,9 @@ func InitConfig(withDefaults bool) {
 	namespacePredefined.AddKnownKey(coreconfig.NamespaceDescription)
 	namespacePredefined.AddKnownKey(coreconfig.NamespaceRemoteName)
 	namespacePredefined.AddKnownKey(coreconfig.NamespacePlugins)
-	// do we need to set the default mode?
+	namespacePredefined.AddKnownKey(coreconfig.NamespaceMode, "multiparty")
 	if withDefaults {
 		namespaceConfig.AddKnownKey(NamespacePredefined+".0."+coreconfig.NamespaceName, "default")
 		namespaceConfig.AddKnownKey(NamespacePredefined+".0."+coreconfig.NamespaceDescription, "Default predefined namespace")
-		namespaceConfig.AddKnownKey(NamespacePredefined+".0."+coreconfig.NamespaceMode, "multiparty")
 	}
 }

internal/namespace/manager.go

@@ -145,25 +145,35 @@ func (nm *namespaceManager) validateNamespaceConfig(ctx context.Context, name st
 		return err
 	}
 
-	if strings.ToLower(name) == "ff_system" || strings.ToLower(conf.GetString(coreconfig.NamespaceRemoteName)) == "ff_system" {
-		return i18n.NewError(ctx, coremsgs.MsgFFSystemReservedName)
+	if strings.ToLower(name) == core.SystemNamespace || strings.ToLower(conf.GetString(coreconfig.NamespaceRemoteName)) == core.SystemNamespace {
+		return i18n.NewError(ctx, coremsgs.MsgFFSystemReservedName, core.SystemNamespace)
 	}
 
 	mode := conf.GetString(coreconfig.NamespaceMode)
 	plugins := conf.GetStringSlice(coreconfig.NamespacePlugins)
 
-	// If the no plugins are found when querying the config, assume older config file
+	// If no plugins are found when querying the config, assume older config file
 	if len(plugins) == 0 {
-		// Still check to ensure at least one bc, dx, ss, and db plugin exist
-		if len(nm.bcPlugins) == 0 || len(nm.dxPlugins) == 0 || len(nm.ssPlugins) == 0 || len(nm.dbPlugins) == 0 {
-			return i18n.NewError(ctx, coremsgs.MsgNamespaceMultipartyConfiguration, name)
+		for plugin := range nm.bcPlugins {
+			plugins = append(plugins, plugin)
+		}
+
+		for plugin := range nm.dxPlugins {
+			plugins = append(plugins, plugin)
+		}
+
+		for plugin := range nm.ssPlugins {
+			plugins = append(plugins, plugin)
+		}
+
+		for plugin := range nm.dbPlugins {
+			plugins = append(plugins, plugin)
 		}
-		return nil
 	}
 
 	switch mode {
 	// Multiparty is the default mode when none is provided
-	case "multiparty", "":
+	case "multiparty":
 		if err := nm.validateMultiPartyConfig(ctx, name, plugins); err != nil {
 			return err
 		}
@@ -220,7 +230,6 @@ func (nm *namespaceManager) validateMultiPartyConfig(ctx context.Context, name s
 	}
 
 	if !dbPlugin || !ssPlugin || !dxPlugin || !bcPlugin {
-		fmt.Printf("plugins: %+v\n", nm)
 		return i18n.NewError(ctx, coremsgs.MsgNamespaceMultipartyConfiguration, name)
 	}
 

internal/namespace/manager_test.go

@@ -19,6 +19,7 @@ package namespace
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/hyperledger/firefly-common/pkg/config"
@@ -35,6 +36,7 @@ import (
 	"github.com/hyperledger/firefly/pkg/dataexchange"
 	"github.com/hyperledger/firefly/pkg/sharedstorage"
 	"github.com/hyperledger/firefly/pkg/tokens"
+	"github.com/spf13/viper"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 )
@@ -236,23 +238,23 @@ func TestInitNamespacesMultipartyMultipleDB(t *testing.T) {
 	assert.Regexp(t, "FF10394", err)
 }
 
-func TestInitNamespacesDeprecatedConfig(t *testing.T) {
+func TestInitNamespacesDeprecatedConfigMultipleBlockchains(t *testing.T) {
 	nm := newTestNamespaceManager(true)
 	defer nm.cleanup(t)
 
 	utTestConfig := config.RootSection("default")
 	utTestConfig.AddKnownKey(coreconfig.NamespaceName, "default")
 	utTestConfig.AddKnownKey(coreconfig.NamespaceRemoteName, "default")
-	utTestConfig.AddKnownKey(coreconfig.NamespaceMode)
+	utTestConfig.AddKnownKey(coreconfig.NamespaceMode, "multiparty")
 	utTestConfig.AddKnownKey(coreconfig.NamespaceDescription)
 	utTestConfig.AddKnownKey(coreconfig.NamespacePlugins, []string{})
 	nm.nsConfig = map[string]config.Section{"default": utTestConfig}
 
-	nm.mdi.On("GetNamespace", mock.Anything, mock.Anything).Return(nil, nil)
-	nm.mdi.On("UpsertNamespace", mock.Anything, mock.Anything, true).Return(nil)
+	// nm.mdi.On("GetNamespace", mock.Anything, mock.Anything).Return(nil, nil)
+	// nm.mdi.On("UpsertNamespace", mock.Anything, mock.Anything, true).Return(nil)
 
 	err := nm.initNamespaces(context.Background(), nm.mdi)
-	assert.NoError(t, err)
+	assert.Regexp(t, "FF10394", err)
 }
 
 func TestInitNamespacesGatewayMultipleDB(t *testing.T) {
@@ -315,26 +317,6 @@ func TestInitNamespacesGatewayWithDX(t *testing.T) {
 	assert.Regexp(t, "FF10393", err)
 }
 
-func TestInitNamespacesDeprecatedConfigNoPlugins(t *testing.T) {
-	nm := newTestNamespaceManager(true)
-	defer nm.cleanup(t)
-
-	utTestConfig := config.RootSection("default")
-	utTestConfig.AddKnownKey(coreconfig.NamespaceName, "default")
-	utTestConfig.AddKnownKey(coreconfig.NamespaceRemoteName, "default")
-	utTestConfig.AddKnownKey(coreconfig.NamespaceMode)
-	utTestConfig.AddKnownKey(coreconfig.NamespaceDescription)
-	utTestConfig.AddKnownKey(coreconfig.NamespacePlugins, []string{})
-	nm.nsConfig = map[string]config.Section{"default": utTestConfig}
-	nm.bcPlugins = nil
-	nm.dbPlugins = nil
-	nm.ssPlugins = nil
-	nm.dxPlugins = nil
-
-	err := nm.initNamespaces(context.Background(), nm.mdi)
-	assert.Regexp(t, "FF10391", err)
-}
-
 func TestInitNamespacesGatewayWithSharedStorage(t *testing.T) {
 	nm := newTestNamespaceManager(true)
 	defer nm.cleanup(t)
@@ -427,24 +409,42 @@ func TestInitNamespacesDefaultMissing(t *testing.T) {
 
 func TestInitNamespacesDupName(t *testing.T) {
 	coreconfig.Reset()
-	InitConfig(true)
-
-	namespaceConfig.AddKnownKey("predefined.0.name", "ns1")
-	namespaceConfig.AddKnownKey("predefined.0.remoteName", "ns1")
-	namespaceConfig.AddKnownKey("predefined.0.mode", "gateway")
-	namespaceConfig.AddKnownKey("predefined.0.plugins", []string{"ethereum", "postgres", "erc721"})
-
-	namespaceConfig.AddKnownKey("predefined.1.name", "ns2")
-	namespaceConfig.AddKnownKey("predefined.1.remoteName", "ns2")
-	namespaceConfig.AddKnownKey("predefined.1.mode", "gateway")
-	namespaceConfig.AddKnownKey("predefined.1.plugins", []string{"ethereum", "postgres", "erc721"})
-
-	namespaceConfig.AddKnownKey("predefined.2.name", "ns2")
-	namespaceConfig.AddKnownKey("predefined.2.remoteName", "ns2")
-	namespaceConfig.AddKnownKey("predefined.2.mode", "gateway")
-	namespaceConfig.AddKnownKey("predefined.2.plugins", []string{"ethereum", "postgres", "erc721"})
-
-	config.Set(coreconfig.NamespacesDefault, "ns1")
+	InitConfig(false)
+
+	viper.SetConfigType("yaml")
+	err := viper.ReadConfig(strings.NewReader(`
+  namespaces:
+    default: ns1
+    predefined:
+    - name: ns1
+      remoteName: ns1
+      mode: gateway
+      org:
+        key: 0x123456
+      plugins:
+      - sqlite3
+      - ethereum
+      - erc721
+    - name: ns2
+      remoteName: ns2
+      mode: gateway
+      org:
+        key: 0x223456
+      plugins:
+      - sqlite3
+      - ethereum
+      - erc721
+    - name: ns2
+      remoteName: ns2
+      mode: gateway
+      org:
+        key: 0x223456
+      plugins:
+      - sqlite3
+      - ethereum
+      - erc721
+  `))
+	assert.NoError(t, err)
 
 	nm := newTestNamespaceManager(false)
 	defer nm.cleanup(t)

internal/orchestrator/orchestrator.go

@@ -176,6 +176,8 @@ type orchestrator struct {
 	sharedDownload       shareddownload.Manager
 	txHelper             txcommon.Helper
 	namespace            namespace.Manager
+	// Used to detect duplicate plugin names
+	pluginNames map[string]bool
 }
 
 func NewOrchestrator(withDefaults bool) Orchestrator {
@@ -358,6 +360,7 @@ func (or *orchestrator) initDatabasePlugins(ctx context.Context, plugins []datab
 		}
 		name := config.GetString(coreconfig.PluginConfigName)
 		or.databases[name] = plugin
+		or.databases[name] = plugin
 
 		if or.database == nil {
 			or.database = plugin
@@ -369,16 +372,21 @@ func (or *orchestrator) initDatabasePlugins(ctx context.Context, plugins []datab
 
 func (or *orchestrator) validatePluginConfig(ctx context.Context, config config.Section, sectionName string) error {
 	name := config.GetString(coreconfig.PluginConfigName)
-	dxType := config.GetString(coreconfig.PluginConfigType)
+	pluginType := config.GetString(coreconfig.PluginConfigType)
 
-	if name == "" || dxType == "" {
+	if name == "" || pluginType == "" {
 		return i18n.NewError(ctx, coremsgs.MsgInvalidPluginConfiguration, sectionName)
 	}
 
 	if err := core.ValidateFFNameField(ctx, name, "name"); err != nil {
 		return err
 	}
 
+	if _, ok := or.pluginNames[name]; ok {
+		return i18n.NewError(ctx, coremsgs.MsgDuplicatePluginName, name)
+	}
+	or.pluginNames[name] = true
+
 	return nil
 }
 
@@ -444,6 +452,7 @@ func (or *orchestrator) initDataExchange(ctx context.Context) (err error) {
 }
 
 func (or *orchestrator) initPlugins(ctx context.Context) (err error) {
+	or.pluginNames = make(map[string]bool)
 	if or.metrics == nil {
 		or.metrics = metrics.NewMetricsManager(ctx)
 	}

internal/orchestrator/orchestrator_test.go

@@ -218,6 +218,7 @@ func TestBadDeprecatedDatabaseInitFail(t *testing.T) {
 
 func TestDatabaseGetPlugins(t *testing.T) {
 	or := newTestOrchestrator()
+	or.pluginNames = make(map[string]bool)
 	defer or.cleanup(t)
 	difactory.InitConfig(databaseConfig)
 	config.Set("plugins.database", []fftypes.JSONObject{{}})
@@ -231,6 +232,7 @@ func TestDatabaseGetPlugins(t *testing.T) {
 
 func TestDatabaseUnknownPlugin(t *testing.T) {
 	or := newTestOrchestrator()
+	or.pluginNames = make(map[string]bool)
 	defer or.cleanup(t)
 	difactory.InitConfig(databaseConfig)
 	config.Set("plugins.database", []fftypes.JSONObject{{}})
@@ -244,6 +246,7 @@ func TestDatabaseUnknownPlugin(t *testing.T) {
 
 func TestDatabaseGetPluginsNoName(t *testing.T) {
 	or := newTestOrchestrator()
+	or.pluginNames = make(map[string]bool)
 	defer or.cleanup(t)
 	difactory.InitConfig(databaseConfig)
 	config.Set("plugins.database", []fftypes.JSONObject{{}})
@@ -416,6 +419,7 @@ func TestDeprecatedBlockchainInitFail(t *testing.T) {
 
 func TestBlockchainGetPlugins(t *testing.T) {
 	or := newTestOrchestrator()
+	or.pluginNames = make(map[string]bool)
 	defer or.cleanup(t)
 	bifactory.InitConfig(blockchainConfig)
 	config.Set("plugins.blockchain", []fftypes.JSONObject{{}})
@@ -747,6 +751,19 @@ func TestBadTokensPlugin(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestDuplicatePluginNames(t *testing.T) {
+	or := newTestOrchestrator()
+	or.pluginNames = make(map[string]bool)
+	or.pluginNames["duplicate"] = true
+	tifactory.InitConfig(tokensConfig)
+	utConfig := config.RootSection("test")
+	utConfig.AddKnownKey(coreconfig.PluginConfigName, "duplicate")
+	utConfig.AddKnownKey(coreconfig.PluginConfigType, "fftokens")
+	ctx := context.Background()
+	err := or.validatePluginConfig(ctx, utConfig, "test")
+	assert.Regexp(t, "FF10395", err)
+}
+
 func TestGoodTokensPlugin(t *testing.T) {
 	or := newTestOrchestrator()
 	defer or.cleanup(t)
@@ -1098,6 +1115,7 @@ func TestInitDataExchangeGetNodesFail(t *testing.T) {
 
 func TestInitDataExchangeWithNodes(t *testing.T) {
 	or := newTestOrchestrator()
+	or.pluginNames = make(map[string]bool)
 	defer or.cleanup(t)
 	or.database = or.mdi
 	dxfactory.InitConfig(dataexchangeConfig)