[FAB-5818] Make MSPConfigHandler immutable

Although the MSPConfigHandler has not followed the configtx value
interface for some time, it still follows the transactional model of
Being/Propose/Precommit/Commit.

This is wrong for a number of reasons now:
  1. The underlying MSP Manager is immutable
  2. The MSPConfigHandler is only used by immutable config

This CR removes all of the mutable components of the code, drastically
simplifying the implementation and reducing the bug surface.

Change-Id: Ia940facbc152b2b17c944df8a2d24c080eff3013
Signed-off-by: Jason Yellick <jyellick@us.ibm.com>

Author: Jason Yellick

common/config/channel/channel.go

@@ -13,6 +13,7 @@ import (
 	"github.com/hyperledger/fabric/bccsp"
 	"github.com/hyperledger/fabric/common/config/channel/msp"
 	"github.com/hyperledger/fabric/common/util"
+	outermsp "github.com/hyperledger/fabric/msp"
 	cb "github.com/hyperledger/fabric/protos/common"
 
 	"github.com/pkg/errors"
@@ -64,7 +65,7 @@ type ChannelConfig struct {
 
 	hashingAlgorithm func(input []byte) []byte
 
-	mspConfigHandler *msp.MSPConfigHandler
+	mspManager outermsp.MSPManager
 
 	appConfig         *ApplicationConfig
 	ordererConfig     *OrdererConfig
@@ -74,10 +75,11 @@ type ChannelConfig struct {
 // NewChannelConfig creates a new ChannelConfig
 func NewChannelConfig(channelGroup *cb.ConfigGroup) (*ChannelConfig, error) {
 	cc := &ChannelConfig{
-		protos:           &ChannelProtos{},
-		mspConfigHandler: msp.NewMSPConfigHandler(),
+		protos: &ChannelProtos{},
 	}
 
+	mspConfigHandler := msp.NewMSPConfigHandler()
+
 	if err := DeserializeProtoValuesFromGroup(channelGroup, cc.protos); err != nil {
 		return nil, errors.Wrap(err, "failed to deserialize values")
 	}
@@ -86,17 +88,15 @@ func NewChannelConfig(channelGroup *cb.ConfigGroup) (*ChannelConfig, error) {
 		return nil, err
 	}
 
-	cc.mspConfigHandler.BeginConfig("")
-
+	var err error
 	for groupName, group := range channelGroup.Groups {
-		var err error
 		switch groupName {
 		case ApplicationGroupKey:
-			cc.appConfig, err = NewApplicationConfig(group, cc.mspConfigHandler)
+			cc.appConfig, err = NewApplicationConfig(group, mspConfigHandler)
 		case OrdererGroupKey:
-			cc.ordererConfig, err = NewOrdererConfig(group, cc.mspConfigHandler)
+			cc.ordererConfig, err = NewOrdererConfig(group, mspConfigHandler)
 		case ConsortiumsGroupKey:
-			cc.consortiumsConfig, err = NewConsortiumsConfig(group, cc.mspConfigHandler)
+			cc.consortiumsConfig, err = NewConsortiumsConfig(group, mspConfigHandler)
 		default:
 			return nil, fmt.Errorf("Disallowed channel group: %s", group)
 		}
@@ -105,17 +105,16 @@ func NewChannelConfig(channelGroup *cb.ConfigGroup) (*ChannelConfig, error) {
 		}
 	}
 
-	if err := cc.mspConfigHandler.PreCommit(""); err != nil {
+	if cc.mspManager, err = mspConfigHandler.CreateMSPManager(); err != nil {
 		return nil, err
 	}
-	cc.mspConfigHandler.CommitProposals("")
 
 	return cc, nil
 }
 
 // MSPManager returns the MSP manager for this config
-func (cc *ChannelConfig) MSPManager() *msp.MSPConfigHandler {
-	return cc.mspConfigHandler
+func (cc *ChannelConfig) MSPManager() outermsp.MSPManager {
+	return cc.mspManager
 }
 
 // OrdererConfig returns the orderer config associated with this channel

common/config/channel/msp/config.go

@@ -1,96 +1,39 @@
 /*
-Copyright IBM Corp. 2017 All Rights Reserved.
+Copyright IBM Corp. All Rights Reserved.
 
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-		 http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
+SPDX-License-Identifier: Apache-2.0
 */
 
 package msp
 
 import (
 	"fmt"
-	"reflect"
-	"sync"
 
 	"github.com/hyperledger/fabric/msp"
 	"github.com/hyperledger/fabric/msp/cache"
 	mspprotos "github.com/hyperledger/fabric/protos/msp"
+
+	"github.com/golang/protobuf/proto"
 )
 
 type pendingMSPConfig struct {
 	mspConfig *mspprotos.MSPConfig
 	msp       msp.MSP
 }
 
-type mspConfigStore struct {
-	idMap       map[string]*pendingMSPConfig
-	proposedMgr msp.MSPManager
-}
-
 // MSPConfigHandler
 type MSPConfigHandler struct {
-	pendingConfig map[interface{}]*mspConfigStore
-	pendingLock   sync.RWMutex
-	msp.MSPManager
+	idMap map[string]*pendingMSPConfig
 }
 
 func NewMSPConfigHandler() *MSPConfigHandler {
 	return &MSPConfigHandler{
-		pendingConfig: make(map[interface{}]*mspConfigStore),
-	}
-}
-
-// BeginConfig called when a config proposal is begun
-func (bh *MSPConfigHandler) BeginConfig(tx interface{}) {
-	bh.pendingLock.Lock()
-	defer bh.pendingLock.Unlock()
-	_, ok := bh.pendingConfig[tx]
-	if ok {
-		panic("Programming error, called BeginConfig multiply for the same tx")
-	}
-	bh.pendingConfig[tx] = &mspConfigStore{
 		idMap: make(map[string]*pendingMSPConfig),
 	}
 }
 
-// RollbackProposals called when a config proposal is abandoned
-func (bh *MSPConfigHandler) RollbackProposals(tx interface{}) {
-	bh.pendingLock.Lock()
-	defer bh.pendingLock.Unlock()
-	delete(bh.pendingConfig, tx)
-}
-
-// CommitProposals called when a config proposal is committed
-func (bh *MSPConfigHandler) CommitProposals(tx interface{}) {
-	bh.pendingLock.Lock()
-	defer bh.pendingLock.Unlock()
-	pendingConfig, ok := bh.pendingConfig[tx]
-	if !ok {
-		panic("Programming error, called BeginConfig multiply for the same tx")
-	}
-
-	bh.MSPManager = pendingConfig.proposedMgr
-	delete(bh.pendingConfig, tx)
-}
-
-// ProposeValue called when config is added to a proposal
-func (bh *MSPConfigHandler) ProposeMSP(tx interface{}, mspConfig *mspprotos.MSPConfig) (msp.MSP, error) {
-	bh.pendingLock.RLock()
-	pendingConfig, ok := bh.pendingConfig[tx]
-	bh.pendingLock.RUnlock()
-	if !ok {
-		panic("Programming error, called BeginConfig multiply for the same tx")
-	}
-
+// ProposeValue called when an org defines an MSP
+func (bh *MSPConfigHandler) ProposeMSP(mspConfig *mspprotos.MSPConfig) (msp.MSP, error) {
 	// check that the type for that MSP is supported
 	if mspConfig.Type != int32(msp.FABRIC) {
 		return nil, fmt.Errorf("Setup error: unsupported msp type %d", mspConfig.Type)
@@ -116,36 +59,30 @@ func (bh *MSPConfigHandler) ProposeMSP(tx interface{}, mspConfig *mspprotos.MSPC
 	// add the MSP to the map of pending MSPs
 	mspID, _ := mspInst.GetIdentifier()
 
-	existingPendingMSPConfig, ok := pendingConfig.idMap[mspID]
-	if ok && !reflect.DeepEqual(existingPendingMSPConfig.mspConfig, mspConfig) {
+	existingPendingMSPConfig, ok := bh.idMap[mspID]
+	if ok && !proto.Equal(existingPendingMSPConfig.mspConfig, mspConfig) {
 		return nil, fmt.Errorf("Attempted to define two different versions of MSP: %s", mspID)
 	}
 
-	pendingConfig.idMap[mspID] = &pendingMSPConfig{
-		mspConfig: mspConfig,
-		msp:       mspInst,
+	if !ok {
+		bh.idMap[mspID] = &pendingMSPConfig{
+			mspConfig: mspConfig,
+			msp:       mspInst,
+		}
 	}
 
 	return mspInst, nil
 }
 
-// PreCommit instantiates the MSP manager
-func (bh *MSPConfigHandler) PreCommit(tx interface{}) error {
-	bh.pendingLock.RLock()
-	pendingConfig, ok := bh.pendingConfig[tx]
-	bh.pendingLock.RUnlock()
-	if !ok {
-		panic("Programming error, called PreCommit for tx which was not started")
-	}
-
-	mspList := make([]msp.MSP, len(pendingConfig.idMap))
+func (bh *MSPConfigHandler) CreateMSPManager() (msp.MSPManager, error) {
+	mspList := make([]msp.MSP, len(bh.idMap))
 	i := 0
-	for _, pendingMSP := range pendingConfig.idMap {
+	for _, pendingMSP := range bh.idMap {
 		mspList[i] = pendingMSP.msp
 		i++
 	}
 
-	pendingConfig.proposedMgr = msp.NewMSPManager()
-	err := pendingConfig.proposedMgr.Setup(mspList)
-	return err
+	manager := msp.NewMSPManager()
+	err := manager.Setup(mspList)
+	return manager, err
 }

common/config/channel/msp/config_test.go

@@ -36,47 +36,36 @@ func TestMSPConfigManager(t *testing.T) {
 
 	// test success:
 
-	// begin/propose/commit
 	mspCH := NewMSPConfigHandler()
 
-	assert.Panics(t, func() {
-		mspCH.PreCommit(t)
-	}, "Expected panic calling PreCommit before beginning transaction")
-	assert.Panics(t, func() {
-		mspCH.CommitProposals(t)
-	}, "Expected panic calling CommitProposals before beginning transaction")
-	assert.Panics(t, func() {
-		_, err = mspCH.ProposeMSP(t, conf)
-	}, "Expected panic calling ProposeMSP before beginning transaction")
+	_, err = mspCH.ProposeMSP(conf)
+	assert.NoError(t, err)
 
-	mspCH.BeginConfig(t)
-	_, err = mspCH.ProposeMSP(t, conf)
+	mgr, err := mspCH.CreateMSPManager()
 	assert.NoError(t, err)
-	mspCH.PreCommit(t)
-	mspCH.CommitProposals(t)
+	assert.NotNil(t, mgr)
 
-	msps, err := mspCH.GetMSPs()
+	msps, err := mgr.GetMSPs()
 	assert.NoError(t, err)
 
 	if msps == nil || len(msps) == 0 {
 		t.Fatalf("There are no MSPS in the manager")
 	}
+}
 
-	mspCH.BeginConfig(t)
-	_, err = mspCH.ProposeMSP(t, conf)
-	mspCH.RollbackProposals(t)
+func TestMSPConfigFailure(t *testing.T) {
+	mspCH := NewMSPConfigHandler()
 
-	// test failure
 	// begin/propose/commit
-	mspCH.BeginConfig(t)
-	_, err = mspCH.ProposeMSP(t, conf)
-	assert.NoError(t, err)
-	_, err = mspCH.ProposeMSP(t, &mspprotos.MSPConfig{Config: []byte("BARF!")})
-	assert.Error(t, err)
-	_, err = mspCH.ProposeMSP(t, &mspprotos.MSPConfig{Type: int32(10)})
-	assert.Panics(t, func() {
-		mspCH.BeginConfig(t)
-	}, "Expected panic calling BeginConfig multiple times for same transaction")
+	t.Run("Bad proto", func(t *testing.T) {
+		_, err := mspCH.ProposeMSP(&mspprotos.MSPConfig{Config: []byte("BARF!")})
+		assert.Error(t, err)
+	})
+
+	t.Run("Bad MSP Type", func(t *testing.T) {
+		_, err := mspCH.ProposeMSP(&mspprotos.MSPConfig{Type: int32(10)})
+		assert.Error(t, err)
+	})
 }
 
 func TestTemplates(t *testing.T) {

common/config/channel/organization.go

@@ -80,7 +80,7 @@ func (oc *OrganizationConfig) validateMSP() error {
 	var err error
 
 	logger.Debugf("Setting up MSP for org %s", oc.name)
-	oc.msp, err = oc.mspConfigHandler.ProposeMSP("", oc.protos.MSP)
+	oc.msp, err = oc.mspConfigHandler.ProposeMSP(oc.protos.MSP)
 	if err != nil {
 		return err
 	}

msp/mgmt/deserializer_test.go

@@ -21,7 +21,6 @@ import (
 	"os"
 	"testing"
 
-	msp2 "github.com/hyperledger/fabric/common/config/channel/msp"
 	"github.com/hyperledger/fabric/core/config"
 	"github.com/hyperledger/fabric/msp"
 	"github.com/stretchr/testify/assert"
@@ -87,7 +86,7 @@ func TestMain(m *testing.M) {
 		os.Exit(-1)
 	}
 
-	XXXSetMSPManager("foo", &msp2.MSPConfigHandler{MSPManager: msp.NewMSPManager()})
+	XXXSetMSPManager("foo", msp.NewMSPManager())
 	retVal := m.Run()
 	os.Exit(retVal)
 }

msp/mgmt/mgmt.go

@@ -23,7 +23,6 @@ import (
 	"errors"
 
 	"github.com/hyperledger/fabric/bccsp/factory"
-	configvaluesmsp "github.com/hyperledger/fabric/common/config/channel/msp"
 	"github.com/hyperledger/fabric/common/flogging"
 	"github.com/hyperledger/fabric/core/config"
 	"github.com/hyperledger/fabric/msp"
@@ -76,24 +75,12 @@ func GetManagerForChain(chainID string) msp.MSPManager {
 		mspMgr = msp.NewMSPManager()
 		mspMap[chainID] = mspMgr
 	} else {
-		switch mgr := mspMgr.(type) {
-		case *configvaluesmsp.MSPConfigHandler:
-			// check for nil MSPManager interface as it can exist but not be
-			// instantiated
-			if mgr.MSPManager == nil {
-				mspLogger.Debugf("MSPManager is not instantiated; no MSPs are defined for this channel.")
-				// return nil so the MSPManager methods cannot be accidentally called,
-				// which would result in a panic
-				return nil
-			}
-		default:
-			// check for internal mspManagerImpl type. if a different type is found,
-			// it's because a developer has added a new type that implements the
-			// MSPManager interface and should add a case to the logic above to handle
-			// it.
-			if reflect.TypeOf(mgr).Elem().Name() != "mspManagerImpl" {
-				panic("Found unexpected MSPManager type.")
-			}
+		// check for internal mspManagerImpl type. if a different type is found,
+		// it's because a developer has added a new type that implements the
+		// MSPManager interface and should add a case to the logic above to handle
+		// it.
+		if reflect.TypeOf(mspMgr).Elem().Name() != "mspManagerImpl" {
+			panic("Found unexpected MSPManager type.")
 		}
 		mspLogger.Debugf("Returning existing manager for channel '%s'", chainID)
 	}

msp/mgmt/mgmt_test.go

@@ -19,7 +19,6 @@ package mgmt
 import (
 	"testing"
 
-	configvaluesmsp "github.com/hyperledger/fabric/common/config/channel/msp"
 	"github.com/hyperledger/fabric/msp"
 	"github.com/stretchr/testify/assert"
 )
@@ -41,14 +40,7 @@ func TestGetManagerForChains(t *testing.T) {
 }
 
 func TestGetManagerForChains_usingMSPConfigHandlers(t *testing.T) {
-	XXXSetMSPManager("test", &configvaluesmsp.MSPConfigHandler{MSPManager: nil})
-	msp1 := GetManagerForChain("test")
-	// return value should be nil because the MSPManager was not initialized
-	if msp1 != nil {
-		t.Fatal("MSPManager should have been nil")
-	}
-
-	XXXSetMSPManager("foo", &configvaluesmsp.MSPConfigHandler{MSPManager: msp.NewMSPManager()})
+	XXXSetMSPManager("foo", msp.NewMSPManager())
 	msp2 := GetManagerForChain("foo")
 	// return value should be set because the MSPManager was initialized
 	if msp2 == nil {
@@ -57,7 +49,7 @@ func TestGetManagerForChains_usingMSPConfigHandlers(t *testing.T) {
 }
 
 func TestGetIdentityDeserializer(t *testing.T) {
-	XXXSetMSPManager("baz", &configvaluesmsp.MSPConfigHandler{MSPManager: msp.NewMSPManager()})
+	XXXSetMSPManager("baz", msp.NewMSPManager())
 	ids := GetIdentityDeserializer("baz")
 	assert.NotNil(t, ids)
 	ids = GetIdentityDeserializer("")