From 6eab9cf6bda377943d7cb09adfe133778d0d1222 Mon Sep 17 00:00:00 2001 From: Jason Yellick Date: Tue, 25 Jul 2017 15:41:58 -0400 Subject: [PATCH] [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 --- common/configtx/manager.go | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/common/configtx/manager.go b/common/configtx/manager.go index c9ff4c081b4..393c5ab3ad5 100644 --- a/common/configtx/manager.go +++ b/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 }