[FAB-2554] configtx.Manager track deserialized val

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

The configtx Manager currently calls into the config deserialization but
does not retain the unmarshaled config values (as they are retained and
accessible through the config).

In order for the configtx manager to also support debugging and
inspection of configuration, it is necessary that these deserialized
versions be tracked by the manager. This CR adds this functionality.

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

Author: Jason Yellick

common/config/api.go

@@ -21,7 +21,6 @@ package config
 import (
 	"time"
 
-	cb "github.com/hyperledger/fabric/protos/common"
 	ab "github.com/hyperledger/fabric/protos/orderer"
 	pb "github.com/hyperledger/fabric/protos/peer"
 )
@@ -86,10 +85,7 @@ type Orderer interface {
 
 type ValueProposer interface {
 	// BeginValueProposals called when a config proposal is begun
-	BeginValueProposals(tx interface{}, groups []string) ([]ValueProposer, error)
-
-	// ProposeValue called when config is added to a proposal
-	ProposeValue(tx interface{}, key string, configValue *cb.ConfigValue) error
+	BeginValueProposals(tx interface{}, groups []string) (ValueDeserializer, []ValueProposer, error)
 
 	// RollbackProposals called when a config proposal is abandoned
 	RollbackProposals(tx interface{})

common/config/proposer.go

@@ -20,24 +20,27 @@ import (
 	"fmt"
 	"sync"
 
-	cb "github.com/hyperledger/fabric/protos/common"
-
 	"github.com/golang/protobuf/proto"
 	logging "github.com/op/go-logging"
 )
 
 var logger = logging.MustGetLogger("common/config")
 
-// ValuesDeserializer provides a mechanism to retrieve proto messages to deserialize config values into
-type ValuesDeserializer interface {
-	// ProtoMsg behaves like a map lookup for key
-	ProtoMsg(key string) (proto.Message, bool)
+// ValueDeserializer provides a mechanism to retrieve proto messages to deserialize config values into
+type ValueDeserializer interface {
+	// Deserialize takes a Value key as a string, and a marshaled Value value as bytes
+	// and returns the deserialized version of that value.  Note, this function operates
+	// with side effects intended.  Using a ValueDeserializer to deserialize a message will
+	// generally set the value in the Values interface that the ValueDeserializer derived from
+	// Therefore, the proto.Message may be safely discarded, but may be retained for
+	// inspection and or debugging purposes.
+	Deserialize(key string, value []byte) (proto.Message, error)
 }
 
 // Values defines a mechanism to supply messages to unamrshal from config
 // and a mechanism to validate the results
 type Values interface {
-	ValuesDeserializer
+	ValueDeserializer
 
 	// Validate should ensure that the values set into the proto messages are correct
 	// and that the new group values are allowed.  It also includes a tx ID in case cross
@@ -75,7 +78,7 @@ func NewProposer(vh Handler) *Proposer {
 }
 
 // BeginValueProposals called when a config proposal is begun
-func (p *Proposer) BeginValueProposals(tx interface{}, groups []string) ([]ValueProposer, error) {
+func (p *Proposer) BeginValueProposals(tx interface{}, groups []string) (ValueDeserializer, []ValueProposer, error) {
 	p.pendingLock.Lock()
 	defer p.pendingLock.Unlock()
 	if _, ok := p.pending[tx]; ok {
@@ -104,7 +107,7 @@ func (p *Proposer) BeginValueProposals(tx interface{}, groups []string) ([]Value
 			group, err = p.vh.NewGroup(groupName)
 			if err != nil {
 				pending = nil
-				return nil, fmt.Errorf("Error creating group %s: %s", groupName, err)
+				return nil, nil, fmt.Errorf("Error creating group %s: %s", groupName, err)
 			}
 		}
 
@@ -114,28 +117,7 @@ func (p *Proposer) BeginValueProposals(tx interface{}, groups []string) ([]Value
 
 	p.pending[tx] = pending
 
-	return result, nil
-}
-
-// ProposeValue called when config is added to a proposal
-func (p *Proposer) ProposeValue(tx interface{}, key string, configValue *cb.ConfigValue) error {
-	p.pendingLock.RLock()
-	pending, ok := p.pending[tx]
-	p.pendingLock.RUnlock()
-	if !ok {
-		logger.Panicf("Serious Programming Error: attempted to propose value for tx which had not been begun")
-	}
-
-	msg, ok := pending.allocated.ProtoMsg(key)
-	if !ok {
-		return fmt.Errorf("Unknown value key %s for %T", key, p.vh)
-	}
-
-	if err := proto.Unmarshal(configValue.Value, msg); err != nil {
-		return fmt.Errorf("Error unmarshaling key to proto message: %s", err)
-	}
-
-	return nil
+	return pending.allocated, result, nil
 }
 
 // Validate ensures that the new config values is a valid change

common/config/proposer_test.go

@@ -37,9 +37,17 @@ type mockValues struct {
 	ValidateReturn error
 }
 
-func (v *mockValues) ProtoMsg(key string) (proto.Message, bool) {
+func (v *mockValues) Deserialize(key string, value []byte) (proto.Message, error) {
 	msg, ok := v.ProtoMsgMap[key]
-	return msg, ok
+	if !ok {
+		return nil, fmt.Errorf("Missing message key: %s", key)
+	}
+	err := proto.Unmarshal(value, msg)
+	if err != nil {
+		return nil, err
+	}
+
+	return msg, nil
 }
 
 func (v *mockValues) Validate(interface{}, map[string]ValueProposer) error {
@@ -110,39 +118,43 @@ func TestGoodKeys(t *testing.T) {
 	mh.AllocateReturn.ProtoMsgMap["Payload"] = &cb.Payload{}
 
 	p := NewProposer(mh)
-	_, err := p.BeginValueProposals(t, nil)
+	vd, _, err := p.BeginValueProposals(t, nil)
 	assert.NoError(t, err)
 
 	env := &cb.Envelope{Payload: []byte("SOME DATA")}
 	pay := &cb.Payload{Data: []byte("SOME OTHER DATA")}
 
-	assert.NoError(t, p.ProposeValue(t, "Envelope", &cb.ConfigValue{Value: utils.MarshalOrPanic(env)}))
-	assert.NoError(t, p.ProposeValue(t, "Payload", &cb.ConfigValue{Value: utils.MarshalOrPanic(pay)}))
+	msg, err := vd.Deserialize("Envelope", utils.MarshalOrPanic(env))
+	assert.NoError(t, err)
+	assert.Equal(t, msg, env)
 
-	assert.Equal(t, mh.AllocateReturn.ProtoMsgMap["Envelope"], env)
-	assert.Equal(t, mh.AllocateReturn.ProtoMsgMap["Payload"], pay)
+	msg, err = vd.Deserialize("Payload", utils.MarshalOrPanic(pay))
+	assert.NoError(t, err)
+	assert.Equal(t, msg, pay)
 }
 
 func TestBadMarshaling(t *testing.T) {
 	mh := newMockHandler()
 	mh.AllocateReturn.ProtoMsgMap["Envelope"] = &cb.Envelope{}
 
 	p := NewProposer(mh)
-	_, err := p.BeginValueProposals(t, nil)
+	vd, _, err := p.BeginValueProposals(t, nil)
 	assert.NoError(t, err)
 
-	assert.Error(t, p.ProposeValue(t, "Envelope", &cb.ConfigValue{Value: []byte("GARBAGE")}), "Should have errored unmarshaling")
+	_, err = vd.Deserialize("Envelope", []byte("GARBAGE"))
+	assert.Error(t, err, "Should have errored unmarshaling")
 }
 
 func TestBadMissingMessage(t *testing.T) {
 	mh := newMockHandler()
 	mh.AllocateReturn.ProtoMsgMap["Payload"] = &cb.Payload{}
 
 	p := NewProposer(mh)
-	_, err := p.BeginValueProposals(t, nil)
+	vd, _, err := p.BeginValueProposals(t, nil)
 	assert.NoError(t, err)
 
-	assert.Error(t, p.ProposeValue(t, "Envelope", &cb.ConfigValue{Value: utils.MarshalOrPanic(&cb.Envelope{})}), "Should have errored on unexpected message")
+	_, err = vd.Deserialize("Envelope", utils.MarshalOrPanic(&cb.Envelope{}))
+	assert.Error(t, err, "Should have errored on unexpected message")
 }
 
 func TestGroups(t *testing.T) {
@@ -151,18 +163,18 @@ func TestGroups(t *testing.T) {
 	mh.NewGroupMap["bar"] = nil
 
 	p := NewProposer(mh)
-	_, err := p.BeginValueProposals(t, []string{"foo", "bar"})
+	_, _, err := p.BeginValueProposals(t, []string{"foo", "bar"})
 	assert.NoError(t, err, "Both groups were present")
 	p.CommitProposals(t)
 
 	mh.NewGroupMap = make(map[string]ValueProposer)
-	_, err = p.BeginValueProposals(t, []string{"foo", "bar"})
+	_, _, err = p.BeginValueProposals(t, []string{"foo", "bar"})
 	assert.NoError(t, err, "Should not have tried to recreate the groups")
 	p.CommitProposals(t)
 
-	_, err = p.BeginValueProposals(t, []string{"foo", "other"})
+	_, _, err = p.BeginValueProposals(t, []string{"foo", "other"})
 	assert.Error(t, err, "Should not have errored when trying to create 'other'")
 
-	_, err = p.BeginValueProposals(t, []string{"foo"})
+	_, _, err = p.BeginValueProposals(t, []string{"foo"})
 	assert.NoError(t, err, "Should be able to begin again without rolling back because of error")
 }

common/config/root.go

@@ -20,7 +20,8 @@ import (
 	"fmt"
 
 	"github.com/hyperledger/fabric/common/config/msp"
-	cb "github.com/hyperledger/fabric/protos/common"
+
+	"github.com/golang/protobuf/proto"
 )
 
 // Root acts as the object which anchors the rest of the config
@@ -39,16 +40,22 @@ func NewRoot(mspConfigHandler *msp.MSPConfigHandler) *Root {
 	}
 }
 
+type failDeserializer struct{}
+
+func (fd failDeserializer) Deserialize(key string, value []byte) (proto.Message, error) {
+	return nil, fmt.Errorf("Programming error, this should never be invoked")
+}
+
 // BeginValueProposals is used to start a new config proposal
-func (r *Root) BeginValueProposals(tx interface{}, groups []string) ([]ValueProposer, error) {
+func (r *Root) BeginValueProposals(tx interface{}, groups []string) (ValueDeserializer, []ValueProposer, error) {
 	if len(groups) != 1 {
-		return nil, fmt.Errorf("Root config only supports having one base group")
+		return nil, nil, fmt.Errorf("Root config only supports having one base group")
 	}
 	if groups[0] != ChannelGroupKey {
-		return nil, fmt.Errorf("Root group must have channel")
+		return nil, nil, fmt.Errorf("Root group must have channel")
 	}
 	r.mspConfigHandler.BeginConfig(tx)
-	return []ValueProposer{r.channel}, nil
+	return failDeserializer{}, []ValueProposer{r.channel}, nil
 }
 
 // RollbackConfig is used to abandon a new config proposal
@@ -66,11 +73,6 @@ func (r *Root) CommitProposals(tx interface{}) {
 	r.mspConfigHandler.CommitProposals(tx)
 }
 
-// ProposeValue should not be invoked on this object
-func (r *Root) ProposeValue(tx interface{}, key string, value *cb.ConfigValue) error {
-	return fmt.Errorf("Programming error, this should never be invoked")
-}
-
 // Channel returns the associated Channel level config
 func (r *Root) Channel() *ChannelGroup {
 	return r.channel

common/config/root_test.go

@@ -32,16 +32,19 @@ func init() {
 func TestBeginBadRoot(t *testing.T) {
 	r := NewRoot(&msp.MSPConfigHandler{})
 
-	_, err := r.BeginValueProposals(t, []string{ChannelGroupKey, ChannelGroupKey})
+	_, _, err := r.BeginValueProposals(t, []string{ChannelGroupKey, ChannelGroupKey})
 	assert.Error(t, err, "Only one root element allowed")
 
-	_, err = r.BeginValueProposals(t, []string{"foo"})
+	_, _, err = r.BeginValueProposals(t, []string{"foo"})
 	assert.Error(t, err, "Non %s group not allowed", ChannelGroupKey)
 }
 
 func TestProposeValue(t *testing.T) {
-	r := NewRoot(&msp.MSPConfigHandler{})
+	r := NewRoot(msp.NewMSPConfigHandler())
+
+	vd, _, err := r.BeginValueProposals(t, []string{ChannelGroupKey})
+	assert.NoError(t, err)
 
-	err := r.ProposeValue(t, "foo", nil)
+	_, err = vd.Deserialize("foo", nil)
 	assert.Error(t, err, "ProposeValue should return error")
 }

common/config/standardvalues.go

@@ -47,9 +47,21 @@ func NewStandardValues(protosStructs ...interface{}) (*standardValues, error) {
 	return sv, nil
 }
 
-func (sv *standardValues) ProtoMsg(key string) (proto.Message, bool) {
+// Deserialize looks up the backing Values proto of the given name, unmarshals the given bytes
+// to populate the backing message structure, and retuns a referenced to the retained deserialized
+// message (or an error, either because the key did not exist, or there was an an error unmarshaling
+func (sv *standardValues) Deserialize(key string, value []byte) (proto.Message, error) {
 	msg, ok := sv.lookup[key]
-	return msg, ok
+	if !ok {
+		return nil, fmt.Errorf("Not found")
+	}
+
+	err := proto.Unmarshal(value, msg)
+	if err != nil {
+		return nil, err
+	}
+
+	return msg, nil
 }
 
 func (sv *standardValues) initializeProtosStruct(objValue reflect.Value) error {

common/config/standardvalues_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	cb "github.com/hyperledger/fabric/protos/common"
+	"github.com/hyperledger/fabric/protos/utils"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -53,12 +54,12 @@ func TestSingle(t *testing.T) {
 	assert.NotNil(t, fooVal.Msg1, "Should have initialized Msg1")
 	assert.NotNil(t, fooVal.Msg2, "Should have initialized Msg2")
 
-	msg1, ok := sv.ProtoMsg("Msg1")
-	assert.True(t, ok, "Should have found map entry")
+	msg1, err := sv.Deserialize("Msg1", utils.MarshalOrPanic(&cb.Envelope{}))
+	assert.NoError(t, err, "Should have found map entry")
 	assert.Equal(t, msg1, fooVal.Msg1, "Should be same entry")
 
-	msg2, ok := sv.ProtoMsg("Msg2")
-	assert.True(t, ok, "Should have found map entry")
+	msg2, err := sv.Deserialize("Msg2", utils.MarshalOrPanic(&cb.Payload{}))
+	assert.NoError(t, err, "Should have found map entry")
 	assert.Equal(t, msg2, fooVal.Msg2, "Should be same entry")
 }
 
@@ -71,20 +72,20 @@ func TestPair(t *testing.T) {
 	assert.NotNil(t, fooVal.Msg2, "Should have initialized Msg2")
 	assert.NotNil(t, barVal.Msg3, "Should have initialized Msg3")
 
-	msg1, ok := sv.ProtoMsg("Msg1")
-	assert.True(t, ok, "Should have found map entry")
+	msg1, err := sv.Deserialize("Msg1", utils.MarshalOrPanic(&cb.Envelope{}))
+	assert.NoError(t, err, "Should have found map entry")
 	assert.Equal(t, msg1, fooVal.Msg1, "Should be same entry")
 
-	msg2, ok := sv.ProtoMsg("Msg2")
-	assert.True(t, ok, "Should have found map entry")
+	msg2, err := sv.Deserialize("Msg2", utils.MarshalOrPanic(&cb.Payload{}))
+	assert.NoError(t, err, "Should have found map entry")
 	assert.Equal(t, msg2, fooVal.Msg2, "Should be same entry")
 
-	msg3, ok := sv.ProtoMsg("Msg3")
-	assert.True(t, ok, "Should have found map entry")
+	msg3, err := sv.Deserialize("Msg3", utils.MarshalOrPanic(&cb.Header{}))
+	assert.NoError(t, err, "Should have found map entry")
 	assert.Equal(t, msg3, barVal.Msg3, "Should be same entry")
 }
 
-func TestNestingConflict(t *testing.T) {
+func TestPairConflict(t *testing.T) {
 	_, err := NewStandardValues(&foo{}, &conflict{})
 	assert.Error(t, err, "Conflicting keys provided")
 }