[FAB-17747] Removal functions should be idempotent (hyperledger#1117)

- Removing values that do not exist in the config will not error
- Remove ConfigGroup existence validation

Signed-off-by: Tiffany Harris <[email protected]>

pkg/configtx/application.go

@@ -111,12 +111,11 @@ func (c *ConfigTx) AddAnchorPeer(orgName string, newAnchorPeer Address) error {
 }
 
 // RemoveAnchorPeer removes an anchor peer from an existing channel config transaction.
-// The removed anchor peer and org it belongs to must both already exist.
+// Specifying an anchor peer or organization name that does not exist in the application
+// ConfigGroup of the channel config will not return an error.
+// Removal will panic if application group or application org group does not exist.
 func (c *ConfigTx) RemoveAnchorPeer(orgName string, anchorPeerToRemove Address) error {
-	applicationOrgGroup, ok := c.updated.ChannelGroup.Groups[ApplicationGroupKey].Groups[orgName]
-	if !ok {
-		return fmt.Errorf("application org %s does not exist in channel config", orgName)
-	}
+	applicationOrgGroup := c.updated.ChannelGroup.Groups[ApplicationGroupKey].Groups[orgName]
 
 	anchorPeersProto := &pb.AnchorPeers{}
 
@@ -129,6 +128,7 @@ func (c *ConfigTx) RemoveAnchorPeer(orgName string, anchorPeerToRemove Address)
 	}
 
 	existingAnchorPeers := anchorPeersProto.AnchorPeers[:0]
+
 	for _, anchorPeer := range anchorPeersProto.AnchorPeers {
 		if anchorPeer.Host != anchorPeerToRemove.Host || anchorPeer.Port != int32(anchorPeerToRemove.Port) {
 			existingAnchorPeers = append(existingAnchorPeers, anchorPeer)
@@ -143,10 +143,6 @@ func (c *ConfigTx) RemoveAnchorPeer(orgName string, anchorPeerToRemove Address)
 		}
 	}
 
-	if len(existingAnchorPeers) == len(anchorPeersProto.AnchorPeers) {
-		return fmt.Errorf("could not find anchor peer %s:%d in application org %s", anchorPeerToRemove.Host, anchorPeerToRemove.Port, orgName)
-	}
-
 	// Add anchor peers config value back to application org
 	err := setValue(applicationOrgGroup, anchorPeersValue(existingAnchorPeers), AdminsPolicyKey)
 	if err != nil {
@@ -157,6 +153,7 @@ func (c *ConfigTx) RemoveAnchorPeer(orgName string, anchorPeerToRemove Address)
 }
 
 // AddACLs adds ACLS to an existing channel config application.
+// Addition will panic if application group does not exist.
 func (c *ConfigTx) AddACLs(acls map[string]string) error {
 	configACLs, err := getACLs(c.updated)
 	if err != nil {
@@ -176,6 +173,8 @@ func (c *ConfigTx) AddACLs(acls map[string]string) error {
 }
 
 // RemoveACLs a list of ACLs from given channel config application.
+// Specifying acls that do not exist in the application ConfigGroup of the channel config will not return a error.
+// Removal will panic if application group does not exist.
 func (c *ConfigTx) RemoveACLs(acls []string) error {
 	configACLs, err := getACLs(c.updated)
 	if err != nil {
@@ -195,16 +194,14 @@ func (c *ConfigTx) RemoveACLs(acls []string) error {
 }
 
 // ApplicationACLs returns a map of application acls from a config transaction.
+// Retrieval will panic if application group does not exist.
 func (c *ConfigTx) ApplicationACLs() (map[string]string, error) {
 	return getACLs(c.original)
 }
 
 // getACLs returns a map of ACLS for given config application.
 func getACLs(config *cb.Config) (map[string]string, error) {
-	applicationGroup, ok := config.ChannelGroup.Groups[ApplicationGroupKey]
-	if !ok {
-		return nil, fmt.Errorf("application does not exist in channel config")
-	}
+	applicationGroup := config.ChannelGroup.Groups[ApplicationGroupKey]
 
 	ACLProtos := &pb.ACLs{}
 
@@ -217,6 +214,7 @@ func getACLs(config *cb.Config) (map[string]string, error) {
 	for apiResource, policyRef := range ACLProtos.Acls {
 		retACLs[apiResource] = policyRef.PolicyRef
 	}
+
 	return retACLs, nil
 }
 

pkg/configtx/application_test.go

@@ -585,19 +585,15 @@ func TestRemoveAnchorPeerFailure(t *testing.T) {
 		testName           string
 		orgName            string
 		anchorPeerToRemove Address
+		configValues       map[string]*cb.ConfigValue
 		expectedErr        string
 	}{
 		{
-			testName:           "When the org for the application does not exist",
-			orgName:            "BadOrg",
-			anchorPeerToRemove: Address{Host: "host1", Port: 123},
-			expectedErr:        "application org BadOrg does not exist in channel config",
-		},
-		{
-			testName:           "When the anchor peer being removed doesn't exist in the org",
+			testName:           "When the unmarshaling existing anchor peer proto fails",
 			orgName:            "Org1",
-			anchorPeerToRemove: Address{Host: "host2", Port: 123},
-			expectedErr:        "could not find anchor peer host2:123 in application org Org1",
+			anchorPeerToRemove: Address{Host: "host1", Port: 123},
+			configValues:       map[string]*cb.ConfigValue{AnchorPeersKey: {Value: []byte("a little fire")}},
+			expectedErr:        "failed unmarshaling Org1's anchor peer endpoints: proto: can't skip unknown wire type 6",
 		},
 	}
 
@@ -613,6 +609,8 @@ func TestRemoveAnchorPeerFailure(t *testing.T) {
 			applicationGroup, err := newApplicationGroup(baseApplicationConf)
 			gt.Expect(err).NotTo(HaveOccurred())
 
+			applicationGroup.Groups["Org1"].Values = tt.configValues
+
 			config := &cb.Config{
 				ChannelGroup: &cb.ConfigGroup{
 					Groups: map[string]*cb.ConfigGroup{
@@ -736,13 +734,6 @@ func TestAddACL(t *testing.T) {
 			},
 			expectedErr: "",
 		},
-		{
-			testName: "configuration missing application config group",
-			configMod: func(config *cb.Config) {
-				delete(config.ChannelGroup.Groups, ApplicationGroupKey)
-			},
-			expectedErr: "application does not exist in channel config",
-		},
 	}
 
 	for _, tt := range tests {
@@ -808,13 +799,6 @@ func TestRemoveACL(t *testing.T) {
 			},
 			expectedErr: "",
 		},
-		{
-			testName: "configuration missing application config group",
-			configMod: func(config *cb.Config) {
-				delete(config.ChannelGroup.Groups, ApplicationGroupKey)
-			},
-			expectedErr: "application does not exist in channel config",
-		},
 	}
 
 	for _, tt := range tests {
@@ -1196,18 +1180,28 @@ func TestApplicationACLsFailure(t *testing.T) {
 
 	gt := NewGomegaWithT(t)
 
+	baseApplicationConf := baseApplication(t)
+	applicationGroup, err := newApplicationGroup(baseApplicationConf)
+	gt.Expect(err).NotTo(HaveOccurred())
+
 	config := &cb.Config{
 		ChannelGroup: &cb.ConfigGroup{
-			Groups: map[string]*cb.ConfigGroup{},
+			Groups: map[string]*cb.ConfigGroup{
+				ApplicationGroupKey: applicationGroup,
+			},
 		},
 	}
 
+	config.ChannelGroup.Groups[ApplicationGroupKey].Values[ACLsKey] = &cb.ConfigValue{
+		Value: []byte("another little fire"),
+	}
+
 	c := &ConfigTx{
 		original: config,
 	}
 
 	applicationACLs, err := c.ApplicationACLs()
-	gt.Expect(err).To(MatchError("application does not exist in channel config"))
+	gt.Expect(err).To(MatchError("unmarshaling ACLs: unexpected EOF"))
 	gt.Expect(applicationACLs).To(BeNil())
 }
 

pkg/configtx/config.go

@@ -582,7 +582,7 @@ func unmarshalConfigValueAtKey(group *cb.ConfigGroup, key string, msg proto.Mess
 
 	err := proto.Unmarshal(valueAtKey.Value, msg)
 	if err != nil {
-		return fmt.Errorf("unmarshalling %s: %v", key, err)
+		return fmt.Errorf("unmarshaling %s: %v", key, err)
 	}
 
 	return nil

pkg/configtx/consortiums.go

@@ -21,17 +21,12 @@ type Consortium struct {
 	Organizations []Organization
 }
 
-// RemoveConsortium remove a consortium from a channel configuration
-func (c *ConfigTx) RemoveConsortium(consortiumName string) error {
+// RemoveConsortium removes a consortium from a channel configuration.
+// Removal will panic if the consortiums group does not exist.
+func (c *ConfigTx) RemoveConsortium(consortiumName string) {
 	consortiumGroup := c.updated.ChannelGroup.Groups[ConsortiumsGroupKey].Groups
-	_, ok := consortiumGroup[consortiumName]
-	if !ok {
-		return fmt.Errorf("consortium %s does not exist in channel config", consortiumName)
-	}
 
 	delete(consortiumGroup, consortiumName)
-
-	return nil
 }
 
 // AddOrgToConsortium adds an org definition to a named consortium in a given

pkg/configtx/consortiums_test.go

@@ -884,41 +884,12 @@ func TestRemoveConsortium(t *testing.T) {
 		updated:  config,
 	}
 
-	err = c.RemoveConsortium("Consortium1")
-	gt.Expect(err).NotTo(HaveOccurred())
+	c.RemoveConsortium("Consortium1")
 
 	updatedConsortiumsGroup := c.updated.ChannelGroup.Groups[ConsortiumsGroupKey]
 	gt.Expect(updatedConsortiumsGroup.Groups["Consortium1"]).To(BeNil())
 }
 
-func TestRemoveConsortiumFailures(t *testing.T) {
-	t.Parallel()
-
-	gt := NewGomegaWithT(t)
-
-	consortiums := baseConsortiums(t)
-	consortiumsGroup, err := newConsortiumsGroup(consortiums)
-	gt.Expect(err).NotTo(HaveOccurred())
-
-	config := &cb.Config{
-		ChannelGroup: &cb.ConfigGroup{
-			Groups: map[string]*cb.ConfigGroup{
-				ConsortiumsGroupKey: consortiumsGroup,
-			},
-			Values:   map[string]*cb.ConfigValue{},
-			Policies: map[string]*cb.ConfigPolicy{},
-		},
-	}
-
-	c := ConfigTx{
-		original: config,
-		updated:  config,
-	}
-
-	err = c.RemoveConsortium("BadConsortium")
-	gt.Expect(err).To(MatchError("consortium BadConsortium does not exist in channel config"))
-}
-
 func TestGetConsortiums(t *testing.T) {
 	t.Parallel()
 	gt := NewGomegaWithT(t)

pkg/configtx/example_test.go

@@ -91,10 +91,7 @@ func Example_systemChannel() {
 		panic(err)
 	}
 
-	err = c.RemoveConsortium("SampleConsortium2")
-	if err != nil {
-		panic(err)
-	}
+	c.RemoveConsortium("SampleConsortium2")
 
 	orgToAdd := configtx.Organization{
 		Name: "Org3",
@@ -124,10 +121,7 @@ func Example_systemChannel() {
 		panic(err)
 	}
 
-	err = c.RemoveConsortiumOrg("SampleConsortium", "Org3")
-	if err != nil {
-		panic(err)
-	}
+	c.RemoveConsortiumOrg("SampleConsortium", "Org3")
 
 	// Compute the delta
 	configUpdate, err := c.ComputeUpdate("testsyschannel")
@@ -385,10 +379,7 @@ func Example_organization() {
 		panic(err)
 	}
 
-	err = c.RemoveApplicationOrg("Org2")
-	if err != nil {
-		panic(err)
-	}
+	c.RemoveApplicationOrg("Org2")
 
 	err = c.AddApplicationOrgPolicy("Org1", configtx.AdminsPolicyKey, "TestPolicy", configtx.Policy{
 		Type: configtx.ImplicitMetaPolicyType,
@@ -413,10 +404,7 @@ func Example_organization() {
 		panic(err)
 	}
 
-	err = c.RemoveOrdererOrg("OrdererOrg2")
-	if err != nil {
-		panic(err)
-	}
+	c.RemoveOrdererOrg("OrdererOrg2")
 
 	err = c.RemoveOrdererOrgPolicy("OrdererOrg", configtx.WritersPolicyKey)
 	if err != nil {

pkg/configtx/orderer.go

@@ -282,19 +282,16 @@ func (c *ConfigTx) AddOrdererEndpoint(orgName string, endpoint Address) error {
 }
 
 // RemoveOrdererEndpoint removes an orderer's endpoint from an existing channel config transaction.
-// The removed endpoint and org it belongs to must both already exist.
+// Removal will panic if either the orderer group or orderer org group does not exist.
 func (c *ConfigTx) RemoveOrdererEndpoint(orgName string, endpoint Address) error {
-	ordererOrgGroup, ok := c.updated.ChannelGroup.Groups[OrdererGroupKey].Groups[orgName]
-	if !ok {
-		return fmt.Errorf("orderer org %s does not exist in channel config", orgName)
-	}
+	ordererOrgGroup := c.updated.ChannelGroup.Groups[OrdererGroupKey].Groups[orgName]
 
 	ordererAddrProto := &cb.OrdererAddresses{}
 
 	if ordererAddrConfigValue, ok := ordererOrgGroup.Values[EndpointsKey]; ok {
 		err := proto.Unmarshal(ordererAddrConfigValue.Value, ordererAddrProto)
 		if err != nil {
-			return fmt.Errorf("failed unmarshalling orderer org %s's endpoints: %v", orgName, err)
+			return fmt.Errorf("failed unmarshaling orderer org %s's endpoints: %v", orgName, err)
 		}
 	}
 
@@ -307,18 +304,13 @@ func (c *ConfigTx) RemoveOrdererEndpoint(orgName string, endpoint Address) error
 		}
 	}
 
-	if len(existingEndpoints) == len(ordererAddrProto.Addresses) {
-		return fmt.Errorf("could not find endpoint %s in orderer org %s", endpointToRemove, orgName)
-	}
-
 	// Add orderer endpoints config value back to orderer org
 	err := setValue(ordererOrgGroup, endpointsValue(existingEndpoints), AdminsPolicyKey)
 	if err != nil {
 		return fmt.Errorf("failed to remove endpoint %v from orderer org %s: %v", endpoint, orgName, err)
 	}
 
 	return nil
-
 }
 
 // newOrdererGroup returns the orderer component of the channel configuration.

pkg/configtx/orderer_test.go

@@ -1854,6 +1854,8 @@ func TestRemoveOrdererEndpoint(t *testing.T) {
 func TestRemoveOrdererEndpointFailure(t *testing.T) {
 	t.Parallel()
 
+	gt := NewGomegaWithT(t)
+
 	config := &cb.Config{
 		ChannelGroup: &cb.ConfigGroup{
 			Groups: map[string]*cb.ConfigGroup{
@@ -1865,9 +1867,7 @@ func TestRemoveOrdererEndpointFailure(t *testing.T) {
 							Values: map[string]*cb.ConfigValue{
 								EndpointsKey: {
 									ModPolicy: AdminsPolicyKey,
-									Value: marshalOrPanic(&cb.OrdererAddresses{
-										Addresses: []string{"127.0.0.1:7050"},
-									}),
+									Value:     []byte("fire time"),
 								},
 							},
 							Policies: map[string]*cb.ConfigPolicy{},
@@ -1888,37 +1888,8 @@ func TestRemoveOrdererEndpointFailure(t *testing.T) {
 		updated:  config,
 	}
 
-	tests := []struct {
-		testName    string
-		orgName     string
-		endpoint    Address
-		expectedErr string
-	}{
-		{
-			testName:    "When the org for the orderer does not exist",
-			orgName:     "BadOrg",
-			endpoint:    Address{Host: "127.0.0.1", Port: 8050},
-			expectedErr: "orderer org BadOrg does not exist in channel config",
-		},
-		{
-			testName:    "When the endpoint being removed does not exist in the org",
-			orgName:     "OrdererOrg",
-			endpoint:    Address{Host: "127.0.0.1", Port: 8050},
-			expectedErr: "could not find endpoint 127.0.0.1:8050 in orderer org OrdererOrg",
-		},
-	}
-
-	for _, tt := range tests {
-		tt := tt
-		t.Run(tt.testName, func(t *testing.T) {
-			t.Parallel()
-
-			gt := NewGomegaWithT(t)
-
-			err := c.RemoveOrdererEndpoint(tt.orgName, tt.endpoint)
-			gt.Expect(err).To(MatchError(tt.expectedErr))
-		})
-	}
+	err := c.RemoveOrdererEndpoint("OrdererOrg", Address{Host: "127.0.0.1", Port: 8050})
+	gt.Expect(err).To(MatchError("failed unmarshaling orderer org OrdererOrg's endpoints: proto: can't skip unknown wire type 6"))
 }
 
 func baseOrdererOfType(t *testing.T, ordererType string) Orderer {