[FAB-10857] Extract discovery endorsement filtering

This change set extracts the logic that deals with discovery
endorsement peer filtering into a separate function, to be used
by the discovery service core for filtering peers according
to collections.

As a result, the code that pre-filters principal sets according
to MSP IDs is removed and the filtering is done implicitly
at the time of layout construction (if no peers with the chaincode
are installed for the given principal, the layout is filtered out in
the isLayoutSatisfied() function).
This removes the customly crafted MSPOfPrincipal method from existence.

Change-Id: I89a3b5142a6893d84a8faf0b65e2fed36829bb7d
Signed-off-by: yacovm <yacovm@il.ibm.com>

Author: yacovm

discovery/client/client_test.go

@@ -671,12 +671,6 @@ func (mdf *ccMetadataFetcher) Metadata(channel string, cc string, _ bool) *chain
 type principalEvaluator struct {
 }
 
-func (pe *principalEvaluator) MSPOfPrincipal(principal *msp.MSPPrincipal) string {
-	sID := &msp.SerializedIdentity{}
-	proto.Unmarshal(principal.Principal, sID)
-	return sID.Mspid
-}
-
 func (pe *principalEvaluator) SatisfiesPrincipal(channel string, identity []byte, principal *msp.MSPPrincipal) error {
 	sID := &msp.SerializedIdentity{}
 	proto.Unmarshal(identity, sID)

discovery/endorsement/endorsement.go

@@ -30,9 +30,6 @@ type principalEvaluator interface {
 	// SatisfiesPrincipal returns whether a given peer identity satisfies a certain principal
 	// on a given channel
 	SatisfiesPrincipal(channel string, identity []byte, principal *msp.MSPPrincipal) error
-
-	// MSPOfPrincipal returns the MSP ID of the given principal
-	MSPOfPrincipal(principal *msp.MSPPrincipal) string
 }
 
 type chaincodeMetadataFetcher interface {
@@ -80,32 +77,17 @@ type peerPrincipalEvaluator func(member NetworkMember, principal *msp.MSPPrincip
 
 // PeersForEndorsement returns an EndorsementDescriptor for a given set of peers, channel, and chaincode
 func (ea *endorsementAnalyzer) PeersForEndorsement(chainID common.ChainID, interest *discovery.ChaincodeInterest) (*discovery.EndorsementDescriptor, error) {
-	identities := ea.IdentityInfo()
-	identitiesByID := identities.ByID()
-	metadataAndCollectionFilters, err := loadMetadataAndFilters(metadataAndFilterContext{
-		identityInfoByID: identitiesByID,
-		interest:         interest,
-		chainID:          chainID,
-		evaluator:        ea,
-		fetch:            ea,
-	})
+	chanMembership, err := ea.PeersAuthorizedByCriteria(chainID, interest)
 	if err != nil {
 		return nil, errors.WithStack(err)
 	}
-	metadata := metadataAndCollectionFilters.md
-	// Filter out peers that don't have the chaincode installed on them
-	chanMembership := ea.PeersOfChannel(chainID).Filter(peersWithChaincode(metadata...))
-	// Filter out peers that aren't authorized by the collection configs of the chaincode invocation chain
-	chanMembership = chanMembership.Filter(metadataAndCollectionFilters.isMemberAuthorized)
 	channelMembersById := chanMembership.ByID()
 	// Choose only the alive messages of those that have joined the channel
 	aliveMembership := ea.Peers().Intersect(chanMembership)
 	membersById := aliveMembership.ByID()
 	// Compute a mapping between the PKI-IDs of members to their identities
-
-	identitiesOfMembers := computeIdentitiesOfMembers(identities, membersById)
-	filter := ea.excludeIfCCNotInstalled(membersById, identitiesByID)
-	principalsSets, err := ea.computePrincipalSets(chainID, interest, filter)
+	identitiesOfMembers := computeIdentitiesOfMembers(ea.IdentityInfo(), membersById)
+	principalsSets, err := ea.computePrincipalSets(chainID, interest)
 	if err != nil {
 		logger.Warningf("Principal set computation failed: %v", err)
 		return nil, errors.WithStack(err)
@@ -121,6 +103,30 @@ func (ea *endorsementAnalyzer) PeersForEndorsement(chainID common.ChainID, inter
 	})
 }
 
+func (ea *endorsementAnalyzer) PeersAuthorizedByCriteria(chainID common.ChainID, interest *discovery.ChaincodeInterest) (Members, error) {
+	peersOfChannel := ea.PeersOfChannel(chainID)
+	if interest == nil || len(interest.Chaincodes) == 0 {
+		return peersOfChannel, nil
+	}
+	identities := ea.IdentityInfo()
+	identitiesByID := identities.ByID()
+	metadataAndCollectionFilters, err := loadMetadataAndFilters(metadataAndFilterContext{
+		identityInfoByID: identitiesByID,
+		interest:         interest,
+		chainID:          chainID,
+		evaluator:        ea,
+		fetch:            ea,
+	})
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+	metadata := metadataAndCollectionFilters.md
+	// Filter out peers that don't have the chaincode installed on them
+	chanMembership := peersOfChannel.Filter(peersWithChaincode(metadata...))
+	// Filter out peers that aren't authorized by the collection configs of the chaincode invocation chain
+	return chanMembership.Filter(metadataAndCollectionFilters.isMemberAuthorized), nil
+}
+
 type context struct {
 	chaincode           string
 	channel             string
@@ -161,23 +167,7 @@ func (ea *endorsementAnalyzer) computeEndorsementResponse(ctx *context) (*discov
 	}, nil
 }
 
-type principalFilter func(policies.PrincipalSet) bool
-
-func (ea *endorsementAnalyzer) excludeIfCCNotInstalled(membersById map[string]NetworkMember, identitiesByID map[string]api.PeerIdentityInfo) principalFilter {
-	// Obtain the MSP IDs of the members of the channel that are alive
-	mspIDsOfChannelPeers := mspIDsOfMembers(membersById, identitiesByID)
-	// Create an exclusion filter for MSP Principals which their peers don't have the chaincode installed
-	excludeMSPsWithoutChaincodeInstalled := func(principal *msp.MSPPrincipal) bool {
-		mspID := ea.MSPOfPrincipal(principal)
-		_, exists := mspIDsOfChannelPeers[mspID]
-		return mspID != "" && exists
-	}
-	return func(principalsSet policies.PrincipalSet) bool {
-		return principalsSet.ContainingOnly(excludeMSPsWithoutChaincodeInstalled)
-	}
-}
-
-func (ea *endorsementAnalyzer) computePrincipalSets(chainID common.ChainID, interest *discovery.ChaincodeInterest, filter principalFilter) (policies.PrincipalSets, error) {
+func (ea *endorsementAnalyzer) computePrincipalSets(chainID common.ChainID, interest *discovery.ChaincodeInterest) (policies.PrincipalSets, error) {
 	var inquireablePolicies []policies.InquireablePolicy
 	for _, chaincode := range interest.Chaincodes {
 		pol := ea.PolicyByChaincode(string(chainID), chaincode.Name)
@@ -193,10 +183,6 @@ func (ea *endorsementAnalyzer) computePrincipalSets(chainID common.ChainID, inte
 	for _, policy := range inquireablePolicies {
 		var cmpsets inquire.ComparablePrincipalSets
 		for _, ps := range policy.SatisfiedBy() {
-			if !filter(ps) {
-				logger.Debug(ps, "filtered out due to chaincodes not being installed on the corresponding organizations")
-				continue
-			}
 			cps := inquire.NewComparablePrincipalSet(ps)
 			if cps == nil {
 				return nil, errors.New("failed creating a comparable principal set")
@@ -549,16 +535,6 @@ func peersWithChaincode(metadata ...*chaincode.Metadata) func(member NetworkMemb
 	}
 }
 
-func mspIDsOfMembers(membersById map[string]NetworkMember, identitiesByID map[string]api.PeerIdentityInfo) map[string]struct{} {
-	res := make(map[string]struct{})
-	for pkiID := range membersById {
-		if identity, exists := identitiesByID[string(pkiID)]; exists {
-			res[string(identity.Organization)] = struct{}{}
-		}
-	}
-	return res
-}
-
 func mergePrincipalSets(cpss []inquire.ComparablePrincipalSets) (inquire.ComparablePrincipalSets, error) {
 	// Obtain the first ComparablePrincipalSet first
 	var cps inquire.ComparablePrincipalSets

discovery/endorsement/endorsement_test.go

@@ -40,18 +40,12 @@ var pkiID2MSPID = map[string]string{
 	"p10": "Org10MSP",
 	"p11": "Org11MSP",
 	"p12": "Org12MSP",
+	"p13": "Org13MSP",
+	"p14": "Org14MSP",
+	"p15": "Org15MSP",
 }
 
 func TestPeersForEndorsement(t *testing.T) {
-	peerRole := func(pkiID string) *msp.MSPPrincipal {
-		return &msp.MSPPrincipal{
-			PrincipalClassification: msp.MSPPrincipal_ROLE,
-			Principal: utils.MarshalOrPanic(&msp.MSPRole{
-				MspIdentifier: pkiID2MSPID[pkiID],
-				Role:          msp.MSPRole_PEER,
-			}),
-		}
-	}
 	extractPeers := func(desc *discoveryprotos.EndorsementDescriptor) map[string]struct{} {
 		res := make(map[string]struct{})
 		for _, endorsers := range desc.EndorsersByGroups {
@@ -183,7 +177,7 @@ func TestPeersForEndorsement(t *testing.T) {
 		analyzer := NewEndorsementAnalyzer(g, pf, &principalEvaluatorMock{}, mf)
 		desc, err := analyzer.PeersForEndorsement(channel, &discoveryprotos.ChaincodeInterest{Chaincodes: []*discoveryprotos.ChaincodeCall{{Name: cc}}})
 		assert.Nil(t, desc)
-		assert.Equal(t, err.Error(), "chaincode isn't installed on sufficient organizations required by the endorsement policy")
+		assert.Equal(t, "cannot satisfy any principal combination", err.Error())
 
 		// Scenario VI: Policy is found, there are enough peers to satisfy policy combinations,
 		// but some peers have the wrong chaincode version, and some don't even have it installed.
@@ -203,22 +197,22 @@ func TestPeersForEndorsement(t *testing.T) {
 		}).Once()
 		desc, err = analyzer.PeersForEndorsement(channel, &discoveryprotos.ChaincodeInterest{Chaincodes: []*discoveryprotos.ChaincodeCall{{Name: cc}}})
 		assert.Nil(t, desc)
-		assert.Equal(t, err.Error(), "chaincode isn't installed on sufficient organizations required by the endorsement policy")
+		assert.Equal(t, "cannot satisfy any principal combination", err.Error())
 	})
 
 	t.Run("NoChaincodeMetadataFromLedger", func(t *testing.T) {
 		// Scenario VII: Policy is found, there are enough peers to satisfy the policy,
 		// but the chaincode metadata cannot be fetched from the ledger.
-		//g.On("PeersOfChannel").Return(chanPeers.toMembers()).Once()
 		pb := principalBuilder{}
 		policy := pb.newSet().addPrincipal(peerRole("p0")).addPrincipal(peerRole("p6")).
 			newSet().addPrincipal(peerRole("p12")).buildPolicy()
+		g.On("PeersOfChannel").Return(chanPeers.toMembers()).Once()
 		pf.On("PolicyByChaincode", cc).Return(policy).Once()
 		mf.On("Metadata").Return(nil).Once()
 		analyzer := NewEndorsementAnalyzer(g, pf, &principalEvaluatorMock{}, mf)
 		desc, err := analyzer.PeersForEndorsement(channel, &discoveryprotos.ChaincodeInterest{Chaincodes: []*discoveryprotos.ChaincodeCall{{Name: cc}}})
 		assert.Nil(t, desc)
-		assert.Equal(t, err.Error(), "No metadata was found for chaincode chaincode in channel test")
+		assert.Equal(t, "No metadata was found for chaincode chaincode in channel test", err.Error())
 	})
 
 	t.Run("Collections", func(t *testing.T) {
@@ -328,6 +322,113 @@ func TestPeersForEndorsement(t *testing.T) {
 	})
 }
 
+func TestPeersAuthorizedByCriteria(t *testing.T) {
+	cc1 := "cc1"
+	cc2 := "cc2"
+	members := peerSet{
+		newPeer(0).withChaincode(cc1, "1.0"),
+		newPeer(3).withChaincode(cc1, "1.0"),
+		newPeer(6).withChaincode(cc1, "1.0"),
+		newPeer(9).withChaincode(cc1, "1.0"),
+		newPeer(12).withChaincode(cc1, "1.0"),
+	}.toMembers()
+
+	members2 := append(discovery.Members{}, members...)
+	members2 = append(members2, peerSet{newPeer(13).withChaincode(cc1, "1.1").withChaincode(cc2, "1.0")}.toMembers()...)
+	members2 = append(members2, peerSet{newPeer(14).withChaincode(cc1, "1.1")}.toMembers()...)
+	members2 = append(members2, peerSet{newPeer(15).withChaincode(cc2, "1.0")}.toMembers()...)
+
+	alivePeers := peerSet{
+		newPeer(0),
+		newPeer(2),
+		newPeer(4),
+		newPeer(6),
+		newPeer(8),
+		newPeer(10),
+		newPeer(11),
+		newPeer(12),
+		newPeer(13),
+		newPeer(14),
+		newPeer(15),
+	}.toMembers()
+
+	identities := identitySet(pkiID2MSPID)
+
+	for _, tst := range []struct {
+		name                 string
+		arguments            *discoveryprotos.ChaincodeInterest
+		totalExistingMembers discovery.Members
+		metadata             []*chaincode.Metadata
+		expected             discovery.Members
+	}{
+		{
+			name:                 "Nil interest",
+			arguments:            nil,
+			totalExistingMembers: members,
+			expected:             members,
+		},
+		{
+			name:                 "Empty interest invocation chain",
+			arguments:            &discoveryprotos.ChaincodeInterest{},
+			totalExistingMembers: members,
+			expected:             members,
+		},
+		{
+			name: "Chaincodes only installed on some peers",
+			arguments: &discoveryprotos.ChaincodeInterest{
+				Chaincodes: []*discoveryprotos.ChaincodeCall{
+					{Name: cc1}, {Name: cc2},
+				},
+			},
+			totalExistingMembers: members2,
+			metadata: []*chaincode.Metadata{{
+				Name: "cc1", Version: "1.1",
+			}, {
+				Name: "cc2", Version: "1.0",
+			}},
+			expected: peerSet{newPeer(13).withChaincode(cc1, "1.1").withChaincode(cc2, "1.0")}.toMembers(),
+		},
+		{
+			name: "Only some peers authorized by collection",
+			arguments: &discoveryprotos.ChaincodeInterest{
+				Chaincodes: []*discoveryprotos.ChaincodeCall{
+					{Name: cc1, CollectionNames: []string{"collection"}},
+				},
+			},
+			totalExistingMembers: members,
+			metadata: []*chaincode.Metadata{{
+				Name: cc1, Version: "1.0",
+				CollectionsConfig: buildCollectionConfig(map[string][]*msp.MSPPrincipal{
+					"collection": {
+						peerRole("p0"),
+						peerRole("p12"),
+					},
+				}),
+			}},
+			expected: peerSet{
+				newPeer(0).withChaincode(cc1, "1.0"),
+				newPeer(12).withChaincode(cc1, "1.0")}.toMembers(),
+		},
+	} {
+		t.Run(tst.name, func(t *testing.T) {
+			g := &gossipMock{}
+			pf := &policyFetcherMock{}
+			mf := &metadataFetcher{}
+			g.On("Peers").Return(alivePeers)
+			g.On("IdentityInfo").Return(identities)
+			g.On("PeersOfChannel").Return(tst.totalExistingMembers).Once()
+			for _, md := range tst.metadata {
+				mf.On("Metadata").Return(md).Once()
+			}
+
+			analyzer := NewEndorsementAnalyzer(g, pf, &principalEvaluatorMock{}, mf)
+			actualMembers, err := analyzer.PeersAuthorizedByCriteria(common.ChainID("mychannel"), tst.arguments)
+			assert.NoError(t, err)
+			assert.Equal(t, tst.expected, actualMembers)
+		})
+	}
+}
+
 func TestPop(t *testing.T) {
 	slice := []inquire.ComparablePrincipalSets{{}, {}}
 	assert.Len(t, slice, 2)
@@ -354,10 +455,7 @@ func TestComputePrincipalSetsNoPolicies(t *testing.T) {
 		Chaincodes: []*discoveryprotos.ChaincodeCall{},
 	}
 	ea := &endorsementAnalyzer{}
-	acceptAll := func(policies.PrincipalSet) bool {
-		return true
-	}
-	_, err := ea.computePrincipalSets(common.ChainID("mychannel"), interest, acceptAll)
+	_, err := ea.computePrincipalSets(common.ChainID("mychannel"), interest)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "no principal sets remained after filtering")
 }
@@ -482,6 +580,16 @@ func newPeer(i int) *peerInfo {
 	}
 }
 
+func peerRole(pkiID string) *msp.MSPPrincipal {
+	return &msp.MSPPrincipal{
+		PrincipalClassification: msp.MSPPrincipal_ROLE,
+		Principal: utils.MarshalOrPanic(&msp.MSPRole{
+			MspIdentifier: pkiID2MSPID[pkiID],
+			Role:          msp.MSPRole_PEER,
+		}),
+	}
+}
+
 func (pi *peerInfo) withChaincode(name, version string) *peerInfo {
 	if pi.Properties == nil {
 		pi.Properties = &gossip.Properties{}
@@ -553,12 +661,6 @@ func (ip inquireablePolicy) SatisfiedBy() []policies.PrincipalSet {
 type principalEvaluatorMock struct {
 }
 
-func (pe *principalEvaluatorMock) MSPOfPrincipal(principal *msp.MSPPrincipal) string {
-	role := &msp.MSPRole{}
-	proto.Unmarshal(principal.Principal, role)
-	return role.MspIdentifier
-}
-
 func (pe *principalEvaluatorMock) SatisfiesPrincipal(channel string, identity []byte, principal *msp.MSPPrincipal) error {
 	peerRole := &msp.MSPRole{}
 	if err := proto.Unmarshal(principal.Principal, peerRole); err != nil {