[FAB-2261] Make Handler creation transactional

https://jira.hyperledger.org/browse/FAB-2261

At the organization level, handlers were being instaniated dynamically,
but, the handlers were only being instructed to begin transactions
statically.  This caused a crash when a dynamically created org handler
would be told to process a config item before begin the config
transaction.

This CR reworks the handler instantiation to be done at the beginning of
a transaction.

Change-Id: Ifbd2f9adf5809cb9fca3108573f105db4b0d790a
Signed-off-by: Jason Yellick <[email protected]>

Author: Jason Yellick

common/cauthdsl/policy_test.go

@@ -57,7 +57,7 @@ func makePolicySource(policyResult bool) *cb.Policy {
 }
 
 func addPolicy(manager *policies.ManagerImpl, id string, policy *cb.Policy) {
-	manager.BeginConfig()
+	manager.BeginConfig(nil)
 	err := manager.ProposePolicy(id, []string{}, &cb.ConfigPolicy{
 		Policy: policy,
 	})

common/configtx/api/api.go

@@ -82,6 +82,8 @@ type OrdererConfig interface {
 // Handler provides a hook which allows other pieces of code to participate in config proposals
 // TODO, this should probably be renamed to ValueHandler
 type Handler interface {
+	Transactional
+
 	// ProposeConfig called when config is added to a proposal
 	ProposeConfig(key string, configValue *cb.ConfigValue) error
 }
@@ -129,7 +131,7 @@ type Resources interface {
 // Transactional is an interface which allows for an update to be proposed and rolled back
 type Transactional interface {
 	// BeginConfig called when a config proposal is begun
-	BeginConfig()
+	BeginConfig(groups []string) ([]Handler, error)
 
 	// RollbackConfig called when a config proposal is abandoned
 	RollbackConfig()
@@ -138,14 +140,6 @@ type Transactional interface {
 	CommitConfig()
 }
 
-// SubInitializer is used downstream from initializer
-type SubInitializer interface {
-	Transactional
-
-	// Handler returns the associated value handler for a given config path
-	Handler(path []string) (Handler, error)
-}
-
 // PolicyHandler is used for config updates to policy
 type PolicyHandler interface {
 	Transactional
@@ -156,7 +150,7 @@ type PolicyHandler interface {
 // Initializer is used as indirection between Manager and Handler to allow
 // for single Handlers to handle multiple paths
 type Initializer interface {
-	SubInitializer
+	Handler
 
 	Resources
 

common/configtx/handlers/application/organization.go

@@ -19,6 +19,7 @@ package application
 import (
 	"fmt"
 
+	"github.com/hyperledger/fabric/common/configtx/api"
 	"github.com/hyperledger/fabric/common/configtx/handlers"
 	mspconfig "github.com/hyperledger/fabric/common/configtx/handlers/msp"
 	cb "github.com/hyperledger/fabric/protos/common"
@@ -62,18 +63,23 @@ func (oc *ApplicationOrgConfig) AnchorPeers() []*pb.AnchorPeer {
 }
 
 // BeginConfig is used to start a new config proposal
-func (oc *ApplicationOrgConfig) BeginConfig() {
+func (oc *ApplicationOrgConfig) BeginConfig(groups []string) ([]api.Handler, error) {
 	logger.Debugf("Beginning a possible new org config")
+	if len(groups) != 0 {
+		return nil, fmt.Errorf("ApplicationGroup does not support subgroups")
+	}
 	if oc.pendingConfig != nil {
 		logger.Panicf("Programming error, cannot call begin in the middle of a proposal")
 	}
 	oc.pendingConfig = &applicationOrgConfig{}
+	return oc.OrgConfig.BeginConfig(groups)
 }
 
 // RollbackConfig is used to abandon a new config proposal
 func (oc *ApplicationOrgConfig) RollbackConfig() {
 	logger.Debugf("Rolling back proposed org config")
 	oc.pendingConfig = nil
+	oc.OrgConfig.RollbackConfig()
 }
 
 // CommitConfig is used to commit a new config proposal
@@ -84,6 +90,7 @@ func (oc *ApplicationOrgConfig) CommitConfig() {
 	}
 	oc.config = oc.pendingConfig
 	oc.pendingConfig = nil
+	oc.OrgConfig.CommitConfig()
 }
 
 // ProposeConfig is used to add new config to the config proposal

common/configtx/handlers/application/organization_test.go

@@ -47,13 +47,13 @@ func groupToKeyValue(configGroup *cb.ConfigGroup) (string, *cb.ConfigValue) {
 }
 
 func TestApplicationOrgInterface(t *testing.T) {
-	_ = configtxapi.SubInitializer(NewApplicationOrgConfig("id", nil))
+	_ = configtxapi.Handler(NewApplicationOrgConfig("id", nil))
 }
 
 func TestApplicationOrgDoubleBegin(t *testing.T) {
 	m := NewApplicationOrgConfig("id", nil)
-	m.BeginConfig()
-	assert.Panics(t, m.BeginConfig, "Two begins back to back should have caused a panic")
+	m.BeginConfig(nil)
+	assert.Panics(t, func() { m.BeginConfig(nil) }, "Two begins back to back should have caused a panic")
 }
 
 func TestApplicationOrgCommitWithoutBegin(t *testing.T) {
@@ -76,7 +76,7 @@ func TestApplicationOrgAnchorPeers(t *testing.T) {
 	invalidMessage := makeInvalidConfigValue()
 	validMessage := TemplateAnchorPeers("id", endVal)
 	m := NewApplicationOrgConfig("id", nil)
-	m.BeginConfig()
+	m.BeginConfig(nil)
 
 	assert.Error(t, m.ProposeConfig(AnchorPeersKey, invalidMessage), "Should have failed on invalid message")
 	assert.NoError(t, m.ProposeConfig(groupToKeyValue(validMessage)), "Should not have failed on invalid message")

common/configtx/handlers/application/sharedconfig.go

@@ -17,8 +17,6 @@ limitations under the License.
 package application
 
 import (
-	"fmt"
-
 	"github.com/hyperledger/fabric/common/configtx/api"
 	"github.com/hyperledger/fabric/common/configtx/handlers"
 	"github.com/hyperledger/fabric/common/configtx/handlers/msp"
@@ -77,14 +75,24 @@ func NewSharedConfigImpl(mspConfig *msp.MSPConfigHandler) *SharedConfigImpl {
 }
 
 // BeginConfig is used to start a new config proposal
-func (di *SharedConfigImpl) BeginConfig() {
+func (di *SharedConfigImpl) BeginConfig(groups []string) ([]api.Handler, error) {
 	logger.Debugf("Beginning a possible new peer shared config")
 	if di.pendingConfig != nil {
 		logger.Panicf("Programming error, cannot call begin in the middle of a proposal")
 	}
 	di.pendingConfig = &sharedConfig{
 		orgs: make(map[string]api.ApplicationOrgConfig),
 	}
+	orgHandlers := make([]api.Handler, len(groups))
+	for i, group := range groups {
+		org, ok := di.pendingConfig.orgs[group]
+		if !ok {
+			org = NewApplicationOrgConfig(group, di.mspConfig)
+			di.pendingConfig.orgs[group] = org
+		}
+		orgHandlers[i] = org.(*ApplicationOrgConfig)
+	}
+	return orgHandlers, nil
 }
 
 // RollbackConfig is used to abandon a new config proposal
@@ -113,21 +121,3 @@ func (di *SharedConfigImpl) ProposeConfig(key string, configValue *cb.ConfigValu
 func (di *SharedConfigImpl) Organizations() map[string]api.ApplicationOrgConfig {
 	return di.config.orgs
 }
-
-// Handler returns the associated api.Handler for the given path
-func (pm *SharedConfigImpl) Handler(path []string) (api.Handler, error) {
-	if len(path) == 0 {
-		return pm, nil
-	}
-
-	if len(path) > 1 {
-		return nil, fmt.Errorf("Application group allows only one further level of nesting")
-	}
-
-	org, ok := pm.pendingConfig.orgs[path[0]]
-	if !ok {
-		org = NewApplicationOrgConfig(path[0], pm.mspConfig)
-		pm.pendingConfig.orgs[path[0]] = org
-	}
-	return org.(*ApplicationOrgConfig), nil
-}

common/configtx/handlers/application/sharedconfig_test.go

@@ -40,8 +40,8 @@ func TestApplicationDoubleBegin(t *testing.T) {
 	}()
 
 	m := NewSharedConfigImpl(nil)
-	m.BeginConfig()
-	m.BeginConfig()
+	m.BeginConfig(nil)
+	m.BeginConfig(nil)
 }
 
 func TestApplicationCommitWithoutBegin(t *testing.T) {

common/configtx/handlers/channel/sharedconfig.go

@@ -106,11 +106,26 @@ func (pm *SharedConfigImpl) OrdererAddresses() []string {
 }
 
 // BeginConfig is used to start a new config proposal
-func (pm *SharedConfigImpl) BeginConfig() {
+func (pm *SharedConfigImpl) BeginConfig(groups []string) ([]api.Handler, error) {
+	handlers := make([]api.Handler, len(groups))
+
+	for i, group := range groups {
+		switch group {
+		case application.GroupKey:
+			handlers[i] = pm.applicationConfig
+		case orderer.GroupKey:
+			handlers[i] = pm.ordererConfig
+		default:
+			return nil, fmt.Errorf("Disallowed channel group: %s", group)
+		}
+	}
+
 	if pm.pendingConfig != nil {
 		logger.Panicf("Programming error, cannot call begin in the middle of a proposal")
 	}
+
 	pm.pendingConfig = &chainConfig{}
+	return handlers, nil
 }
 
 // RollbackConfig is used to abandon a new config proposal
@@ -163,25 +178,3 @@ func (pm *SharedConfigImpl) ProposeConfig(key string, configValue *cb.ConfigValu
 	}
 	return nil
 }
-
-// Handler returns the associated api.Handler for the given path
-func (pm *SharedConfigImpl) Handler(path []string) (api.Handler, error) {
-	if len(path) == 0 {
-		return pm, nil
-	}
-
-	var initializer api.SubInitializer
-
-	switch path[0] {
-	case application.GroupKey:
-		initializer = pm.applicationConfig
-	case orderer.GroupKey:
-		initializer = pm.ordererConfig
-	default:
-		return nil, fmt.Errorf("Disallowed channel group: %s", path[0])
-	}
-
-	return initializer.Handler(path[1:])
-
-	return nil, fmt.Errorf("Unallowed group")
-}

common/configtx/handlers/channel/sharedconfig_test.go

@@ -55,8 +55,8 @@ func TestDoubleBegin(t *testing.T) {
 	}()
 
 	m := NewSharedConfigImpl(nil, nil)
-	m.BeginConfig()
-	m.BeginConfig()
+	m.BeginConfig(nil)
+	m.BeginConfig(nil)
 }
 
 func TestCommitWithoutBegin(t *testing.T) {
@@ -85,7 +85,7 @@ func TestHashingAlgorithm(t *testing.T) {
 	validAlgorithm := DefaultHashingAlgorithm()
 
 	m := NewSharedConfigImpl(nil, nil)
-	m.BeginConfig()
+	m.BeginConfig(nil)
 
 	err := m.ProposeConfig(HashingAlgorithmKey, invalidMessage)
 	if err == nil {
@@ -115,7 +115,7 @@ func TestBlockDataHashingStructure(t *testing.T) {
 	validWidth := DefaultBlockDataHashingStructure()
 
 	m := NewSharedConfigImpl(nil, nil)
-	m.BeginConfig()
+	m.BeginConfig(nil)
 
 	err := m.ProposeConfig(BlockDataHashingStructureKey, invalidMessage)
 	if err == nil {
@@ -143,7 +143,7 @@ func TestOrdererAddresses(t *testing.T) {
 	invalidMessage := makeInvalidConfigValue()
 	validMessage := DefaultOrdererAddresses()
 	m := NewSharedConfigImpl(nil, nil)
-	m.BeginConfig()
+	m.BeginConfig(nil)
 
 	err := m.ProposeConfig(OrdererAddressesKey, invalidMessage)
 	if err == nil {

common/configtx/handlers/msp/config.go

@@ -21,7 +21,6 @@ import (
 	"reflect"
 
 	"github.com/golang/protobuf/proto"
-	"github.com/hyperledger/fabric/common/configtx/api"
 	"github.com/hyperledger/fabric/msp"
 	"github.com/hyperledger/fabric/protos/common"
 	mspprotos "github.com/hyperledger/fabric/protos/msp"
@@ -83,12 +82,3 @@ func (bh *MSPConfigHandler) ProposeConfig(key string, configValue *common.Config
 	bh.proposedMgr = msp.NewMSPManager()
 	return bh.proposedMgr.Setup(bh.config)
 }
-
-// Handler returns the associated api.Handler for the given path
-func (bh *MSPConfigHandler) Handler(path []string) (api.Handler, error) {
-	if len(path) > 0 {
-		return nil, fmt.Errorf("MSP handler does not support nested groups")
-	}
-
-	return bh, nil
-}

common/configtx/handlers/orderer/sharedconfig.go

@@ -157,14 +157,24 @@ func (pm *ManagerImpl) EgressPolicyNames() []string {
 }
 
 // BeginConfig is used to start a new config proposal
-func (pm *ManagerImpl) BeginConfig() {
-	logger.Debugf("Beginning possible new orderer config")
+func (pm *ManagerImpl) BeginConfig(groups []string) ([]api.Handler, error) {
+	logger.Debugf("Beginning a possible new orderer shared config")
 	if pm.pendingConfig != nil {
-		logger.Fatalf("Programming error, cannot call begin in the middle of a proposal")
+		logger.Panicf("Programming error, cannot call begin in the middle of a proposal")
 	}
 	pm.pendingConfig = &ordererConfig{
 		orgs: make(map[string]*handlers.OrgConfig),
 	}
+	orgHandlers := make([]api.Handler, len(groups))
+	for i, group := range groups {
+		org, ok := pm.pendingConfig.orgs[group]
+		if !ok {
+			org = handlers.NewOrgConfig(group, pm.mspConfig)
+			pm.pendingConfig.orgs[group] = org
+		}
+		orgHandlers[i] = org
+	}
+	return orgHandlers, nil
 }
 
 // RollbackConfig is used to abandon a new config proposal
@@ -275,24 +285,6 @@ func (pm *ManagerImpl) ProposeConfig(key string, configValue *cb.ConfigValue) er
 	return nil
 }
 
-// Handler returns the associated api.Handler for the given path
-func (pm *ManagerImpl) Handler(path []string) (api.Handler, error) {
-	if len(path) == 0 {
-		return pm, nil
-	}
-
-	if len(path) > 1 {
-		return nil, fmt.Errorf("Orderer group allows only one further level of nesting")
-	}
-
-	org, ok := pm.pendingConfig.orgs[path[0]]
-	if !ok {
-		org = handlers.NewOrgConfig(path[0], pm.mspConfig)
-		pm.pendingConfig.orgs[path[0]] = org
-	}
-	return org, nil
-}
-
 // This does just a barebones sanity check.
 func brokerEntrySeemsValid(broker string) bool {
 	if !strings.Contains(broker, ":") {