[FAB-5459] Recompute configmap instead of updating

The configtx validation code works by transforming the ConfigGroup tree
structure into a map, where each config element has a fully qualified
key like:

  [Groups] /Channel/Application

or

  [Policy] /Channel/Application/Readers

This flattened structure makes it easier to check which elements have
been modified, as the maps may simply be iterated over, rather than
walking the config tree.

After applying a config update, the current config map is copied, and
the elements which were updated are overlayed onto the old config.  This
map is then converted back into a tree structure and serialized to be
the new config tree.

The current code adopts the updated config map as the current config
map.  However, this produces the bug described in 5459.  Because the
updated config map is the overlay of the new config onto the old
config, the updated config may contain orphaned nodes which appear in
the map, but which do not appear in the config tree.

Consequently, when a subsequent update arrives, it is validated not
only against the current config, but also against the orphaned nodes
which are still in the updated config map.

This CR changes the logic to discard the updated config map (which may
contain this orphaned nodes) and instead has the config map recomputed
every time a new config is adopted.  This is more obviously
deterministic and mimics the way a new ordering node would initialize
the config after being restarted.

Note: There is no accompanying test case with this.  I had originally
written a test case which demonstrated that nodes were orphaned in the
updated config.  However, this is expected and not a useful test case.
Similarly, forcing the update config map to have updated nodes, then
testing that that map is not adopted does not provide a valuable test
case.

So, instead of a test, this CR opts for some code comments and this
lengthly commit explaining the change.

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

Author: Jason Yellick

common/configtx/manager.go

@@ -204,49 +204,49 @@ func (cm *configManager) proposeConfigUpdate(configtx *cb.Envelope) (*cb.ConfigE
 	}, nil
 }
 
-func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (map[string]comparable, *configResult, error) {
+func (cm *configManager) prepareApply(configEnv *cb.ConfigEnvelope) (*configResult, error) {
 	if configEnv == nil {
-		return nil, nil, fmt.Errorf("Attempted to apply config with nil envelope")
+		return nil, fmt.Errorf("Attempted to apply config with nil envelope")
 	}
 
 	if configEnv.Config == nil {
-		return nil, nil, fmt.Errorf("Config cannot be nil")
+		return nil, fmt.Errorf("Config cannot be nil")
 	}
 
 	if configEnv.Config.Sequence != cm.current.sequence+1 {
-		return nil, nil, fmt.Errorf("Config at sequence %d, cannot prepare to update to %d", cm.current.sequence, configEnv.Config.Sequence)
+		return nil, fmt.Errorf("Config at sequence %d, cannot prepare to update to %d", cm.current.sequence, configEnv.Config.Sequence)
 	}
 
 	configUpdateEnv, err := envelopeToConfigUpdate(configEnv.LastUpdate)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	configMap, err := cm.authorizeUpdate(configUpdateEnv)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
 	channelGroup, err := configMapToConfig(configMap)
 	if err != nil {
-		return nil, nil, fmt.Errorf("Could not turn configMap back to channelGroup: %s", err)
+		return nil, fmt.Errorf("Could not turn configMap back to channelGroup: %s", err)
 	}
 
 	if !reflect.DeepEqual(channelGroup, configEnv.Config.ChannelGroup) {
-		return nil, nil, fmt.Errorf("ConfigEnvelope LastUpdate did not produce the supplied config result")
+		return nil, fmt.Errorf("ConfigEnvelope LastUpdate did not produce the supplied config result")
 	}
 
 	result, err := cm.processConfig(channelGroup)
 	if err != nil {
-		return nil, nil, err
+		return nil, err
 	}
 
-	return configMap, result, nil
+	return result, nil
 }
 
 // Validate simulates applying a ConfigEnvelope to become the new config
 func (cm *configManager) Validate(configEnv *cb.ConfigEnvelope) error {
-	_, result, err := cm.prepareApply(configEnv)
+	result, err := cm.prepareApply(configEnv)
 	if err != nil {
 		return err
 	}
@@ -258,7 +258,17 @@ func (cm *configManager) Validate(configEnv *cb.ConfigEnvelope) error {
 
 // Apply attempts to apply a ConfigEnvelope to become the new config
 func (cm *configManager) Apply(configEnv *cb.ConfigEnvelope) error {
-	configMap, result, err := cm.prepareApply(configEnv)
+	// Note, although prepareApply will necessarilly compute a config map
+	// for the updated config, this config map will possibly contain unreachable
+	// elements from a config graph perspective.  Therefore, it is not safe to use
+	// as the config map after application.  Instead, we compute the config map
+	// just like we would at startup.
+	configMap, err := MapConfig(configEnv.Config.ChannelGroup)
+	if err != nil {
+		return err
+	}
+
+	result, err := cm.prepareApply(configEnv)
 	if err != nil {
 		return err
 	}