From 194e68d19f5d84de11277093ddfabb93f386ba4b Mon Sep 17 00:00:00 2001 From: Jason Yellick Date: Mon, 20 Feb 2017 16:40:41 -0500 Subject: [PATCH] [FAB-2335] Add PreCommit transaction hook https://jira.hyperledger.org/browse/FAB-2335 Today, config transactions go through three phases: BeginValueProposals() ProposeValue(...) ... ProposeValue(...) CommitProposals() This works well for verifying individual components of configuration, but fails when configuration must be validated in its totality. This is exhibited for instance in the instantiation of the MSP manager where the check is performed at every value proposal, rather than only once as is necessary. So, the new path becomes: BeginValueProposals() ProposeValue(...) ... ProposeValue(...) PreCommit() CommitProposals() Where PreCommit may return an error and stop the commit process. This concept also works nicely with some other pending config work intended to remove a significant amount of duplicated code. Change-Id: Ibf7450e6b6e355690af2cb14278f0fce4efdf0d7 Signed-off-by: Jason Yellick --- common/configtx/api/api.go | 3 ++ common/configtx/config.go | 16 ++++++++++ common/configtx/initializer.go | 11 +++++++ common/configvalues/api.go | 5 +++ .../channel/application/organization.go | 3 ++ .../channel/application/sharedconfig.go | 3 ++ .../common/organization/organization.go | 3 ++ common/configvalues/channel/config.go | 3 ++ .../channel/orderer/sharedconfig.go | 3 ++ common/configvalues/msp/config.go | 31 ++++++++++--------- common/configvalues/msp/config_test.go | 1 + common/mocks/configtx/configtx.go | 3 ++ common/policies/policy.go | 12 +++++++ 13 files changed, 83 insertions(+), 14 deletions(-) diff --git a/common/configtx/api/api.go b/common/configtx/api/api.go index 3d6a3d9b6bd..23d858be11b 100644 --- a/common/configtx/api/api.go +++ b/common/configtx/api/api.go @@ -72,6 +72,9 @@ type Transactional interface { // RollbackConfig called when a config proposal is abandoned RollbackProposals() + // PreCommit verifies that the transaction can be committed successfully + PreCommit() error + // CommitConfig called when a config proposal is committed CommitProposals() } diff --git a/common/configtx/config.go b/common/configtx/config.go index de2cffc3de0..160fc52b561 100644 --- a/common/configtx/config.go +++ b/common/configtx/config.go @@ -31,6 +31,16 @@ type configResult struct { subResults []*configResult } +func (cr *configResult) preCommit() error { + for _, subResult := range cr.subResults { + err := subResult.preCommit() + if err != nil { + return err + } + } + return cr.handler.PreCommit() +} + func (cr *configResult) commit() { for _, subResult := range cr.subResults { subResult.commit() @@ -103,6 +113,12 @@ func (cm *configManager) proposeGroup(name string, group *cb.ConfigGroup, handle } } + err = result.preCommit() + if err != nil { + result.rollback() + return nil, err + } + return result, nil } diff --git a/common/configtx/initializer.go b/common/configtx/initializer.go index b6e5222ced5..359b868c64b 100644 --- a/common/configtx/initializer.go +++ b/common/configtx/initializer.go @@ -117,6 +117,12 @@ func (i *valueProposerRoot) RollbackProposals() { i.mspConfigHandler.RollbackProposals() } +// PreCommit is used to verify total configuration before commit +func (i *valueProposerRoot) PreCommit() error { + logger.Debugf("Calling pre commit for MSP manager") + return i.mspConfigHandler.PreCommit() +} + // CommitConfig is used to commit a new config proposal func (i *valueProposerRoot) CommitProposals() { logger.Debugf("Calling commit for MSP manager") @@ -139,6 +145,11 @@ func (i *policyProposerRoot) ProposePolicy(key string, policy *cb.ConfigPolicy) return fmt.Errorf("Programming error, this should never be invoked") } +// PreCommit is a no-op and returns nil +func (i *policyProposerRoot) PreCommit() error { + return nil +} + // RollbackConfig is used to abandon a new config proposal func (i *policyProposerRoot) RollbackProposals() {} diff --git a/common/configvalues/api.go b/common/configvalues/api.go index 760dedd6cca..3d64f139d6b 100644 --- a/common/configvalues/api.go +++ b/common/configvalues/api.go @@ -84,6 +84,11 @@ type ValueProposer interface { // RollbackProposals called when a config proposal is abandoned RollbackProposals() + // PreCommit is invoked before committing the config to catch + // any errors which cannot be caught on a per proposal basis + // TODO, rename other methods to remove Value/Proposal references + PreCommit() error + // CommitProposals called when a config proposal is committed CommitProposals() } diff --git a/common/configvalues/channel/application/organization.go b/common/configvalues/channel/application/organization.go index 7879b565c5e..78fa530e23c 100644 --- a/common/configvalues/channel/application/organization.go +++ b/common/configvalues/channel/application/organization.go @@ -111,3 +111,6 @@ func (oc *ApplicationOrgConfig) ProposeValue(key string, configValue *cb.ConfigV return nil } + +// PreCommit returns nil +func (c *ApplicationOrgConfig) PreCommit() error { return nil } diff --git a/common/configvalues/channel/application/sharedconfig.go b/common/configvalues/channel/application/sharedconfig.go index 985466529c7..bca0d788107 100644 --- a/common/configvalues/channel/application/sharedconfig.go +++ b/common/configvalues/channel/application/sharedconfig.go @@ -121,3 +121,6 @@ func (di *SharedConfigImpl) ProposeValue(key string, configValue *cb.ConfigValue func (di *SharedConfigImpl) Organizations() map[string]api.ApplicationOrg { return di.config.orgs } + +// PreCommit returns nil +func (di *SharedConfigImpl) PreCommit() error { return nil } diff --git a/common/configvalues/channel/common/organization/organization.go b/common/configvalues/channel/common/organization/organization.go index 17eb2dea59b..eb0150ae65a 100644 --- a/common/configvalues/channel/common/organization/organization.go +++ b/common/configvalues/channel/common/organization/organization.go @@ -143,3 +143,6 @@ func (oc *OrgConfig) ProposeValue(key string, configValue *cb.ConfigValue) error } return nil } + +// PreCommit returns nil +func (oc *OrgConfig) PreCommit() error { return nil } diff --git a/common/configvalues/channel/config.go b/common/configvalues/channel/config.go index 455b932b20e..4d0063a4352 100644 --- a/common/configvalues/channel/config.go +++ b/common/configvalues/channel/config.go @@ -158,6 +158,9 @@ func (c *Config) CommitProposals() { c.pending = nil } +// PreCommit returns nil +func (c *Config) PreCommit() error { return nil } + // ProposeValue is used to add new config to the config proposal func (c *Config) ProposeValue(key string, configValue *cb.ConfigValue) error { switch key { diff --git a/common/configvalues/channel/orderer/sharedconfig.go b/common/configvalues/channel/orderer/sharedconfig.go index 7cacdff5aa7..41b65cd5bf9 100644 --- a/common/configvalues/channel/orderer/sharedconfig.go +++ b/common/configvalues/channel/orderer/sharedconfig.go @@ -315,3 +315,6 @@ func brokerEntrySeemsValid(broker string) bool { matched := re.FindString(host) return len(matched) == len(host) } + +// PreCommit returns nil +func (pm *ManagerImpl) PreCommit() error { return nil } diff --git a/common/configvalues/msp/config.go b/common/configvalues/msp/config.go index 92e8feba458..dd9e81cd01c 100644 --- a/common/configvalues/msp/config.go +++ b/common/configvalues/msp/config.go @@ -93,11 +93,21 @@ func (bh *MSPConfigHandler) ProposeMSP(mspConfig *mspprotos.MSPConfig) (msp.MSP, existingPendingMSPConfig, ok := bh.pendingConfig.idMap[mspID] if ok && !reflect.DeepEqual(existingPendingMSPConfig.mspConfig, mspConfig) { return nil, fmt.Errorf("Attempted to define two different versions of MSP: %s", mspID) - } else { - bh.pendingConfig.idMap[mspID] = &pendingMSPConfig{ - mspConfig: mspConfig, - msp: mspInst, - } + } + + bh.pendingConfig.idMap[mspID] = &pendingMSPConfig{ + mspConfig: mspConfig, + msp: mspInst, + } + + return mspInst, nil +} + +// PreCommit instantiates the MSP manager +func (bh *MSPConfigHandler) PreCommit() error { + if len(bh.pendingConfig.idMap) == 0 { + // Cannot instantiate an MSP manager with no MSPs + return nil } mspList := make([]msp.MSP, len(bh.pendingConfig.idMap)) @@ -107,14 +117,7 @@ func (bh *MSPConfigHandler) ProposeMSP(mspConfig *mspprotos.MSPConfig) (msp.MSP, i++ } - // the only way to make sure that I have a - // workable config is to toss the proposed - // manager, create a new one, call setup on - // it and return whatever error setup gives me bh.pendingConfig.proposedMgr = msp.NewMSPManager() - err = bh.pendingConfig.proposedMgr.Setup(mspList) - if err != nil { - return nil, err - } - return mspInst, nil + err := bh.pendingConfig.proposedMgr.Setup(mspList) + return err } diff --git a/common/configvalues/msp/config_test.go b/common/configvalues/msp/config_test.go index 52fe6254b02..31bd30cd25a 100644 --- a/common/configvalues/msp/config_test.go +++ b/common/configvalues/msp/config_test.go @@ -35,6 +35,7 @@ func TestMSPConfigManager(t *testing.T) { mspCH.BeginConfig() _, err = mspCH.ProposeMSP(conf) assert.NoError(t, err) + mspCH.PreCommit() mspCH.CommitProposals() msps, err := mspCH.GetMSPs() diff --git a/common/mocks/configtx/configtx.go b/common/mocks/configtx/configtx.go index 29ca7a42a4e..ee6f226fb76 100644 --- a/common/mocks/configtx/configtx.go +++ b/common/mocks/configtx/configtx.go @@ -70,6 +70,9 @@ func (r *Resources) MSPManager() msp.MSPManager { // Transactional implements the configtxapi.Transactional type Transactional struct{} +// PreCommit returns nil +func (t *Transactional) PreCommit() error { return nil } + // CommitConfig does nothing func (t *Transactional) CommitProposals() {} diff --git a/common/policies/policy.go b/common/policies/policy.go index 3060a63cc60..85ac0b6ef52 100644 --- a/common/policies/policy.go +++ b/common/policies/policy.go @@ -54,13 +54,20 @@ type Manager interface { // Proposer is the interface used by the configtx manager for policy management type Proposer interface { + // BeginPolicyProposals starts a policy update transaction BeginPolicyProposals(groups []string) ([]Proposer, error) + // ProposePolicy createss a pending policy update from a ConfigPolicy ProposePolicy(name string, policy *cb.ConfigPolicy) error + // RollbackProposals discards the pending policy updates RollbackProposals() + // CommitProposals commits the pending policy updates CommitProposals() + + // PreCommit tests if a commit will apply + PreCommit() error } // Provider provides the backing implementation of a policy @@ -176,6 +183,11 @@ func (pm *ManagerImpl) RollbackProposals() { pm.pendingConfig = nil } +// PreCommit is currently a no-op for the policy manager and always returns nil +func (pm *ManagerImpl) PreCommit() error { + return nil +} + // CommitProposals is used to commit a new config proposal func (pm *ManagerImpl) CommitProposals() { if pm.pendingConfig == nil {