[FAB-5814] Make policies.Manager immutable

In this change series to make the channel config immutable, up until
now, the focus has been on removing dependencies on the conflation
between the configtx authorization and the config parsing.  The real
payoff however is in removing the complicated state tracking logic from
all of the assorted mutable components.

This CR tackles the policies.ManagerImpl, which traditionally must
maintain a map of pending changes, create multiple implementations of
the manager, then stitch them together into a working hierarchy on
commit.  This is a great example of the simpliciation which comes from
not treating these structures mutably, as the complexity is lowered
substantially.

This also means that the temporary "poor man's policy parsing" may be
removed.

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

common/cauthdsl/policy_test.go

@@ -24,6 +24,7 @@ import (
 	cb "github.com/hyperledger/fabric/protos/common"
 
 	"github.com/golang/protobuf/proto"
+	"github.com/stretchr/testify/assert"
 )
 
 var acceptAllPolicy *cb.Policy
@@ -56,17 +57,6 @@ func makePolicySource(policyResult bool) *cb.Policy {
 	}
 }
 
-func addPolicy(manager policies.Proposer, id string, policy *cb.Policy) {
-	manager.BeginPolicyProposals(id, nil)
-	_, err := manager.ProposePolicy(id, id, &cb.ConfigPolicy{
-		Policy: policy,
-	})
-	if err != nil {
-		panic(err)
-	}
-	manager.CommitProposals(id)
-}
-
 func providerMap() map[int32]policies.Provider {
 	r := make(map[int32]policies.Provider)
 	r[int32(cb.Policy_SIGNATURE)] = NewPolicyProvider(&mockDeserializer{})
@@ -75,40 +65,41 @@ func providerMap() map[int32]policies.Provider {
 
 func TestAccept(t *testing.T) {
 	policyID := "policyID"
-	m := policies.NewManagerImpl("test", providerMap())
-	addPolicy(m, policyID, acceptAllPolicy)
+	m, err := policies.NewManagerImpl("test", providerMap(), &cb.ConfigGroup{
+		Policies: map[string]*cb.ConfigPolicy{
+			policyID: &cb.ConfigPolicy{Policy: acceptAllPolicy},
+		},
+	})
+	assert.NoError(t, err)
+	assert.NotNil(t, m)
+
 	policy, ok := m.GetPolicy(policyID)
-	if !ok {
-		t.Error("Should have found policy which was just added, but did not")
-	}
-	err := policy.Evaluate([]*cb.SignedData{})
-	if err != nil {
-		t.Fatalf("Should not have errored evaluating an acceptAll policy: %s", err)
-	}
+	assert.True(t, ok, "Should have found policy which was just added, but did not")
+	err = policy.Evaluate([]*cb.SignedData{})
+	assert.NoError(t, err, "Should not have errored evaluating an acceptAll policy")
 }
 
 func TestReject(t *testing.T) {
 	policyID := "policyID"
-	m := policies.NewManagerImpl("test", providerMap())
-	addPolicy(m, policyID, rejectAllPolicy)
+	m, err := policies.NewManagerImpl("test", providerMap(), &cb.ConfigGroup{
+		Policies: map[string]*cb.ConfigPolicy{
+			policyID: &cb.ConfigPolicy{Policy: rejectAllPolicy},
+		},
+	})
+	assert.NoError(t, err)
+	assert.NotNil(t, m)
 	policy, ok := m.GetPolicy(policyID)
-	if !ok {
-		t.Error("Should have found policy which was just added, but did not")
-	}
-	err := policy.Evaluate([]*cb.SignedData{})
-	if err == nil {
-		t.Fatal("Should have errored evaluating the rejectAll policy")
-	}
+	assert.True(t, ok, "Should have found policy which was just added, but did not")
+	err = policy.Evaluate([]*cb.SignedData{})
+	assert.Error(t, err, "Should have errored evaluating an rejectAll policy")
 }
 
 func TestRejectOnUnknown(t *testing.T) {
-	m := policies.NewManagerImpl("test", providerMap())
+	m, err := policies.NewManagerImpl("test", providerMap(), &cb.ConfigGroup{})
+	assert.NoError(t, err)
+	assert.NotNil(t, m)
 	policy, ok := m.GetPolicy("FakePolicyID")
-	if ok {
-		t.Error("Should not have found policy which was never added, but did")
-	}
-	err := policy.Evaluate([]*cb.SignedData{})
-	if err == nil {
-		t.Fatal("Should have errored evaluating the default policy")
-	}
+	assert.False(t, ok, "Should not have found policy which was never added, but did")
+	err = policy.Evaluate([]*cb.SignedData{})
+	assert.Error(t, err, "Should have errored evaluating the default policy")
 }

common/channelconfig/bundle.go

@@ -154,8 +154,7 @@ func NewBundle(channelID string, config *cb.Config) (*Bundle, error) {
 		return nil, errors.Wrap(err, "initializing config values failed")
 	}
 
-	policyManager := policies.NewManagerImpl(RootGroupKey, policyProviderMap)
-	err = InitializePolicyManager(policyManager, config.ChannelGroup)
+	policyManager, err := policies.NewManagerImpl(RootGroupKey, policyProviderMap, config.ChannelGroup)
 	if err != nil {
 		return nil, errors.Wrap(err, "initializing policymanager failed")
 	}

common/channelconfig/util.go

@@ -8,44 +8,11 @@ package channelconfig
 
 import (
 	"github.com/hyperledger/fabric/common/config"
-	"github.com/hyperledger/fabric/common/policies"
 	cb "github.com/hyperledger/fabric/protos/common"
 
 	"github.com/pkg/errors"
 )
 
-// InitializePolicyManager takes a config group and uses it to initialize a PolicyManager
-// XXX This goes away by the end of the CR series so no test logic
-func InitializePolicyManager(pm policies.Proposer, group *cb.ConfigGroup) error {
-	subGroups := make([]string, len(group.Groups))
-	i := 0
-	for subGroup := range group.Groups {
-		subGroups[i] = subGroup
-		i++
-	}
-
-	subPolicyHandlers, err := pm.BeginPolicyProposals("", subGroups)
-	if err != nil {
-		return err
-	}
-
-	for key, policy := range group.Policies {
-		_, err := pm.ProposePolicy("", key, policy)
-		if err != nil {
-			return err
-		}
-	}
-
-	for i := range subGroups {
-		if err := InitializePolicyManager(subPolicyHandlers[i], group.Groups[subGroups[i]]); err != nil {
-			return errors.Wrapf(err, "failed initializing subgroup %s", subGroups[i])
-		}
-	}
-
-	pm.CommitProposals("")
-	return nil
-}
-
 // InitializeConfigValues takes a config group and uses it to initialize a config Root
 // XXX This goes away by the end of the CR series so no test logic
 func InitializeConfigValues(vp config.ValueProposer, group *cb.ConfigGroup) error {

common/config/resources/initializer.go

@@ -17,47 +17,20 @@ import (
 	"github.com/hyperledger/fabric/common/policies"
 	"github.com/hyperledger/fabric/msp"
 	cb "github.com/hyperledger/fabric/protos/common"
-
-	"github.com/golang/protobuf/proto"
+	"github.com/hyperledger/fabric/protos/utils"
 )
 
 const RootGroupKey = "Resources"
 
-type policyProposerRoot struct {
-	policyManager *policies.ManagerImpl
-}
-
-// BeginPolicyProposals is used to start a new config proposal
-func (p *policyProposerRoot) BeginPolicyProposals(tx interface{}, groups []string) ([]policies.Proposer, error) {
-	if len(groups) != 1 {
-		logger.Panicf("Initializer only supports having one root group")
-	}
-	return []policies.Proposer{p.policyManager}, nil
-}
-
-func (i *policyProposerRoot) ProposePolicy(tx interface{}, key string, policy *cb.ConfigPolicy) (proto.Message, error) {
-	return nil, fmt.Errorf("Programming error, this should never be invoked")
-}
-
-// PreCommit is a no-op and returns nil
-func (i *policyProposerRoot) PreCommit(tx interface{}) error {
-	return nil
-}
-
-// RollbackConfig is used to abandon a new config proposal
-func (i *policyProposerRoot) RollbackProposals(tx interface{}) {}
-
-// CommitConfig is used to commit a new config proposal
-func (i *policyProposerRoot) CommitProposals(tx interface{}) {}
-
 type Bundle struct {
-	ppr *policyProposerRoot
 	vpr *valueProposerRoot
 	cm  configtxapi.Manager
 	pm  policies.Manager
+	rpm policies.Manager
 }
 
 // New creates a new resources config bundle
+// TODO, change interface to take config and not an envelope
 func New(envConfig *cb.Envelope, mspManager msp.MSPManager, channelPolicyManager policies.Manager) (*Bundle, error) {
 	policyProviderMap := make(map[int32]policies.Provider)
 	for pType := range cb.Policy_PolicyType_name {
@@ -72,31 +45,35 @@ func New(envConfig *cb.Envelope, mspManager msp.MSPManager, channelPolicyManager
 		}
 	}
 
-	b := &Bundle{
-		vpr: newValueProposerRoot(),
-		ppr: &policyProposerRoot{
-			policyManager: policies.NewManagerImpl(RootGroupKey, policyProviderMap),
-		},
-	}
-	b.pm = &policyRouter{
-		channelPolicyManager:   channelPolicyManager,
-		resourcesPolicyManager: b.ppr.policyManager,
+	payload, err := utils.UnmarshalPayload(envConfig.Payload)
+	if err != nil {
+		return nil, err
 	}
 
-	var err error
-	b.cm, err = configtx.NewManagerImpl(envConfig, b)
+	configEnvelope, err := configtx.UnmarshalConfigEnvelope(payload.Data)
 	if err != nil {
 		return nil, err
 	}
-	configEnvelope := b.cm.ConfigEnvelope()
+
 	if configEnvelope.Config == nil || configEnvelope.Config.ChannelGroup == nil {
 		return nil, fmt.Errorf("config is nil")
 	}
-	err = newchannelconfig.InitializePolicyManager(b.ppr, &cb.ConfigGroup{
-		Groups: map[string]*cb.ConfigGroup{
-			RootGroupKey: configEnvelope.Config.ChannelGroup,
+
+	resourcesPolicyManager, err := policies.NewManagerImpl(RootGroupKey, policyProviderMap, configEnvelope.Config.ChannelGroup)
+	if err != nil {
+		return nil, err
+	}
+
+	b := &Bundle{
+		vpr: newValueProposerRoot(),
+		rpm: resourcesPolicyManager,
+		pm: &policyRouter{
+			channelPolicyManager:   channelPolicyManager,
+			resourcesPolicyManager: resourcesPolicyManager,
 		},
-	})
+	}
+
+	b.cm, err = configtx.NewManagerImpl(envConfig, b)
 	if err != nil {
 		return nil, err
 	}
@@ -117,10 +94,6 @@ func (b *Bundle) RootGroupKey() string {
 	return RootGroupKey
 }
 
-func (b *Bundle) PolicyProposer() policies.Proposer {
-	return b.ppr
-}
-
 func (b *Bundle) ValueProposer() config.ValueProposer {
 	return b.vpr
 }

common/config/resources/initializer_test.go

@@ -69,7 +69,6 @@ func TestBundleGreenPath(t *testing.T) {
 
 	t.Run("Code coverage nits", func(t *testing.T) {
 		assert.Equal(t, b.RootGroupKey(), RootGroupKey)
-		assert.NotNil(t, b.PolicyProposer())
 		assert.NotNil(t, b.ValueProposer())
 		assert.NotNil(t, b.ConfigtxManager())
 		assert.NotNil(t, b.PolicyManager())

common/policies/implicitmeta.go

@@ -25,44 +25,45 @@ import (
 )
 
 type implicitMetaPolicy struct {
-	conf        *cb.ImplicitMetaPolicy
 	threshold   int
 	subPolicies []Policy
 }
 
 // NewPolicy creates a new policy based on the policy bytes
-func newImplicitMetaPolicy(data []byte) (*implicitMetaPolicy, error) {
-	imp := &cb.ImplicitMetaPolicy{}
-	if err := proto.Unmarshal(data, imp); err != nil {
+func newImplicitMetaPolicy(data []byte, managers map[string]*ManagerImpl) (*implicitMetaPolicy, error) {
+	definition := &cb.ImplicitMetaPolicy{}
+	if err := proto.Unmarshal(data, definition); err != nil {
 		return nil, fmt.Errorf("Error unmarshaling to ImplicitMetaPolicy: %s", err)
 	}
 
-	return &implicitMetaPolicy{
-		conf: imp,
-	}, nil
-}
+	subPolicies := make([]Policy, len(managers))
 
-func (imp *implicitMetaPolicy) initialize(config *policyConfig) {
-	imp.subPolicies = make([]Policy, len(config.managers))
 	i := 0
-	for _, manager := range config.managers {
-		imp.subPolicies[i], _ = manager.GetPolicy(imp.conf.SubPolicy)
+	for _, manager := range managers {
+		subPolicies[i], _ = manager.GetPolicy(definition.SubPolicy)
 		i++
 	}
 
-	switch imp.conf.Rule {
+	var threshold int
+
+	switch definition.Rule {
 	case cb.ImplicitMetaPolicy_ANY:
-		imp.threshold = 1
+		threshold = 1
 	case cb.ImplicitMetaPolicy_ALL:
-		imp.threshold = len(imp.subPolicies)
+		threshold = len(subPolicies)
 	case cb.ImplicitMetaPolicy_MAJORITY:
-		imp.threshold = len(imp.subPolicies)/2 + 1
+		threshold = len(subPolicies)/2 + 1
 	}
 
 	// In the special case that there are no policies, consider 0 to be a majority or any
-	if len(imp.subPolicies) == 0 {
-		imp.threshold = 0
+	if len(subPolicies) == 0 {
+		threshold = 0
 	}
+
+	return &implicitMetaPolicy{
+		subPolicies: subPolicies,
+		threshold:   threshold,
+	}, nil
 }
 
 // Evaluate takes a set of SignedData and evaluates whether this set of signatures satisfies the policy

common/policies/implicitmeta_test.go

@@ -35,7 +35,7 @@ func (rp acceptPolicy) Evaluate(signedData []*cb.SignedData) error {
 }
 
 func TestImplicitMarshalError(t *testing.T) {
-	_, err := newImplicitMetaPolicy([]byte("GARBAGE"))
+	_, err := newImplicitMetaPolicy([]byte("GARBAGE"), nil)
 	assert.Error(t, err, "Should have errored unmarshaling garbage")
 }
 
@@ -50,9 +50,7 @@ func makeManagers(count, passing int) map[string]*ManagerImpl {
 		remaining--
 
 		result[fmt.Sprintf("%d", i)] = &ManagerImpl{
-			config: &policyConfig{
-				policies: policyMap,
-			},
+			policies: policyMap,
 		}
 	}
 	return result
@@ -63,15 +61,11 @@ func runPolicyTest(rule cb.ImplicitMetaPolicy_Rule, managerCount int, passingCou
 	imp, err := newImplicitMetaPolicy(utils.MarshalOrPanic(&cb.ImplicitMetaPolicy{
 		Rule:      rule,
 		SubPolicy: TestPolicyName,
-	}))
+	}), makeManagers(managerCount, passingCount))
 	if err != nil {
 		panic(err)
 	}
 
-	imp.initialize(&policyConfig{
-		managers: makeManagers(managerCount, passingCount),
-	})
-
 	return imp.Evaluate(nil)
 }