FAB-14735 Ignore unchanged consenters in update

The config update detection code in Raft is a re-implementation of the
fabric config checking mechanisms.  This should probably be changed in
the long term, but one of its currently unhandled cases, is when a
config update references the consenters in the write set, but does not
modify them.  This can occur especially when adding a new organization
to the orderer group.  This CR adds the additional version checks
necessary to detect and ignore these sorts of updates.

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

orderer/consensus/etcdraft/chain_test.go

@@ -486,17 +486,25 @@ var _ = Describe("Chain", func() {
 
 						// use to prepare the Orderer Values
 						BeforeEach(func() {
-							values := map[string]*common.ConfigValue{
+							newValues := map[string]*common.ConfigValue{
 								"BatchTimeout": {
 									Version: 1,
 									Value: marshalOrPanic(&orderer.BatchTimeout{
 										Timeout: "3ms",
 									}),
 								},
+								"ConsensusType": {
+									Version: 4,
+								},
+							}
+							oldValues := map[string]*common.ConfigValue{
+								"ConsensusType": {
+									Version: 4,
+								},
 							}
 							configEnv = newConfigEnv(channelID,
 								common.HeaderType_CONFIG,
-								newConfigUpdateEnv(channelID, values),
+								newConfigUpdateEnv(channelID, oldValues, newValues),
 							)
 							configSeq = 0
 						}) // BeforeEach block
@@ -597,7 +605,7 @@ var _ = Describe("Chain", func() {
 							values := make(map[string]*common.ConfigValue)
 							configEnv = newConfigEnv(chainID,
 								common.HeaderType_CONFIG,
-								newConfigUpdateEnv(chainID, values),
+								newConfigUpdateEnv(chainID, nil, values),
 							)
 							configSeq = 0
 						}) // BeforeEach block
@@ -624,7 +632,7 @@ var _ = Describe("Chain", func() {
 							}
 							configEnv = newConfigEnv(channelID,
 								common.HeaderType_CONFIG,
-								newConfigUpdateEnv(channelID, values))
+								newConfigUpdateEnv(channelID, nil, values))
 							configSeq = 0
 
 						}) // BeforeEach block
@@ -650,7 +658,7 @@ var _ = Describe("Chain", func() {
 							}
 							configEnv = newConfigEnv(channelID,
 								common.HeaderType_CONFIG,
-								newConfigUpdateEnv(channelID, values))
+								newConfigUpdateEnv(channelID, nil, values))
 							configSeq = 0
 
 						}) // BeforeEach block
@@ -685,7 +693,7 @@ var _ = Describe("Chain", func() {
 							}
 							configEnv = newConfigEnv(channelID,
 								common.HeaderType_CONFIG,
-								newConfigUpdateEnv(channelID, values))
+								newConfigUpdateEnv(channelID, nil, values))
 							configSeq = 0
 
 							err := chain.Configure(configEnv, configSeq)
@@ -706,7 +714,7 @@ var _ = Describe("Chain", func() {
 							}
 							configEnv = newConfigEnv(channelID,
 								common.HeaderType_CONFIG,
-								newConfigUpdateEnv(channelID, values))
+								newConfigUpdateEnv(channelID, nil, values))
 							configSeq = 0
 
 							err := chain.Configure(configEnv, configSeq)
@@ -1519,7 +1527,7 @@ var _ = Describe("Chain", func() {
 					}
 
 					By("creating new configuration with removed node and new one")
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, value))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, value))
 					c1.cutter.CutNext = true
 
 					By("sending config transaction")
@@ -1556,7 +1564,7 @@ var _ = Describe("Chain", func() {
 					}
 
 					By("creating new configuration with removed node and new one")
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, value))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, value))
 					c1.cutter.CutNext = true
 
 					By("sending config transaction")
@@ -1571,10 +1579,10 @@ var _ = Describe("Chain", func() {
 						c1.cutter.CutNext = true
 						configEnvAdd := newConfigEnv(channelID,
 							common.HeaderType_CONFIG,
-							newConfigUpdateEnv(channelID, addConsenterConfigValue()))
+							newConfigUpdateEnv(channelID, nil, addConsenterConfigValue()))
 						configEnvRm := newConfigEnv(channelID,
 							common.HeaderType_CONFIG,
-							newConfigUpdateEnv(channelID, removeConsenterConfigValue(3)))
+							newConfigUpdateEnv(channelID, nil, removeConsenterConfigValue(3)))
 
 						By("Submitting two config tx back-to-back")
 						c1.support.SequenceReturnsOnCall(1, 0)
@@ -1592,7 +1600,7 @@ var _ = Describe("Chain", func() {
 
 				It("adding node to the cluster", func() {
 					addConsenterUpdate := addConsenterConfigValue()
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, addConsenterUpdate))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, addConsenterUpdate))
 					c1.cutter.CutNext = true
 
 					By("sending config transaction")
@@ -1652,7 +1660,7 @@ var _ = Describe("Chain", func() {
 					duplicatedMetadata := &raftprotos.ConfigMetadata{}
 					proto.Unmarshal(consensusType.Metadata, duplicatedMetadata)
 					duplicatedMetadata.Consenters = append(duplicatedMetadata.Consenters, duplicatedMetadata.Consenters[1])
-					configEnv = newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, map[string]*common.ConfigValue{
+					configEnv = newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, map[string]*common.ConfigValue{
 						"ConsensusType": {
 							Version: 1,
 							Value: marshalOrPanic(&orderer.ConsensusType{
@@ -1672,7 +1680,7 @@ var _ = Describe("Chain", func() {
 				It("does not reconfigure raft cluster if it's a channel creation tx", func() {
 					configEnv := newConfigEnv("another-channel",
 						common.HeaderType_CONFIG,
-						newConfigUpdateEnv(channelID, removeConsenterConfigValue(2)))
+						newConfigUpdateEnv(channelID, nil, removeConsenterConfigValue(2)))
 
 					// Wrap config env in Orderer transaction
 					channelCreationEnv := &common.Envelope{
@@ -1712,7 +1720,7 @@ var _ = Describe("Chain", func() {
 					// disconnect second node
 					network.disconnect(2)
 
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, addConsenterConfigValue()))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, addConsenterConfigValue()))
 					c1.cutter.CutNext = true
 
 					By("sending config transaction")
@@ -1787,7 +1795,7 @@ var _ = Describe("Chain", func() {
 					// re-configuration. Later we connecting c1 back and making sure it capable of catching up with
 					// new configuration and successfully rejoins replica set.
 
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, addConsenterConfigValue()))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, addConsenterConfigValue()))
 					c1.cutter.CutNext = true
 
 					step1 := c1.getStepFunc()
@@ -1862,7 +1870,7 @@ var _ = Describe("Chain", func() {
 					// configure chain support mock to stop cluster after config block is committed.
 					// Restart the cluster and ensure it picks up updates and capable to finish reconfiguration.
 
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, addConsenterConfigValue()))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, addConsenterConfigValue()))
 					c1.cutter.CutNext = true
 
 					step1 := c1.getStepFunc()
@@ -1942,7 +1950,7 @@ var _ = Describe("Chain", func() {
 
 					configEnv := newConfigEnv(channelID,
 						common.HeaderType_CONFIG,
-						newConfigUpdateEnv(channelID, removeConsenterConfigValue(1))) // remove nodeID == 1
+						newConfigUpdateEnv(channelID, nil, removeConsenterConfigValue(1))) // remove nodeID == 1
 
 					c1.cutter.CutNext = true
 
@@ -1990,7 +1998,7 @@ var _ = Describe("Chain", func() {
 
 					configEnv := newConfigEnv(channelID,
 						common.HeaderType_CONFIG,
-						newConfigUpdateEnv(channelID, removeConsenterConfigValue(1))) // remove nodeID == 1
+						newConfigUpdateEnv(channelID, nil, removeConsenterConfigValue(1))) // remove nodeID == 1
 
 					c1.cutter.CutNext = true
 
@@ -2037,7 +2045,7 @@ var _ = Describe("Chain", func() {
 				})
 
 				It("does not deadlock if leader steps down while config block is in-flight", func() {
-					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, addConsenterConfigValue()))
+					configEnv := newConfigEnv(channelID, common.HeaderType_CONFIG, newConfigUpdateEnv(channelID, nil, addConsenterConfigValue()))
 					c1.cutter.CutNext = true
 
 					signal := make(chan struct{})
@@ -2511,7 +2519,7 @@ var _ = Describe("Chain", func() {
 					}
 					configEnv = newConfigEnv(channelID,
 						common.HeaderType_CONFIG,
-						newConfigUpdateEnv(channelID, values),
+						newConfigUpdateEnv(channelID, nil, values),
 					)
 				})
 
@@ -3460,15 +3468,21 @@ func newConfigEnv(chainID string, headerType common.HeaderType, configUpdateEnv
 	}
 }
 
-func newConfigUpdateEnv(chainID string, values map[string]*common.ConfigValue) *common.ConfigUpdateEnvelope {
+func newConfigUpdateEnv(chainID string, oldValues, newValues map[string]*common.ConfigValue) *common.ConfigUpdateEnvelope {
 	return &common.ConfigUpdateEnvelope{
 		ConfigUpdate: marshalOrPanic(&common.ConfigUpdate{
 			ChannelId: chainID,
-			ReadSet:   &common.ConfigGroup{},
+			ReadSet: &common.ConfigGroup{
+				Groups: map[string]*common.ConfigGroup{
+					"Orderer": {
+						Values: oldValues,
+					},
+				},
+			},
 			WriteSet: &common.ConfigGroup{
 				Groups: map[string]*common.ConfigGroup{
 					"Orderer": {
-						Values: values,
+						Values: newValues,
 					},
 				},
 			}, // WriteSet

orderer/consensus/etcdraft/util.go

@@ -278,9 +278,25 @@ func MetadataFromConfigValue(configValue *common.ConfigValue) (*etcdraft.ConfigM
 
 // MetadataFromConfigUpdate extracts consensus metadata from config update
 func MetadataFromConfigUpdate(update *common.ConfigUpdate) (*etcdraft.ConfigMetadata, error) {
-	if ordererConfigGroup, ok := update.WriteSet.Groups["Orderer"]; ok {
-		if val, ok := ordererConfigGroup.Values["ConsensusType"]; ok {
-			return MetadataFromConfigValue(val)
+	var baseVersion uint64
+	if update.ReadSet != nil && update.ReadSet.Groups != nil {
+		if ordererConfigGroup, ok := update.ReadSet.Groups["Orderer"]; ok {
+			if val, ok := ordererConfigGroup.Values["ConsensusType"]; ok {
+				baseVersion = val.Version
+			}
+		}
+	}
+
+	if update.WriteSet != nil && update.WriteSet.Groups != nil {
+		if ordererConfigGroup, ok := update.WriteSet.Groups["Orderer"]; ok {
+			if val, ok := ordererConfigGroup.Values["ConsensusType"]; ok {
+				if baseVersion == val.Version {
+					// Only if the version in the write set differs from the read-set
+					// should we consider this to be an update to the consensus type
+					return nil, nil
+				}
+				return MetadataFromConfigValue(val)
+			}
 		}
 	}
 	return nil, nil