global fwd and subnet error msgs fix & test cases (#1080)

* global fwd and subnet error msgs fix & test cases

Fixed the error message to indicate correctly the state of vlans and
vxlans instead of printing an empty string instead of a number when 0.

The test cases are testing too many things in one test case, split out
the tests for forward mode and subnet as they are doing more validation.

Signed-off-by: Chris Plock <[email protected]>

* simplified

* err no longer a pointer to error

Author: chrisplo

netmaster/objApi/apiController.go

@@ -22,7 +22,10 @@ import (
 	"strings"
 
 	"encoding/json"
-	"github.com/contiv/contivmodel"
+	"io/ioutil"
+	"net/http"
+
+	contivModel "github.com/contiv/contivmodel"
 	"github.com/contiv/netplugin/core"
 	"github.com/contiv/netplugin/drivers"
 	"github.com/contiv/netplugin/netmaster/docknet"
@@ -34,8 +37,6 @@ import (
 	"github.com/contiv/netplugin/objdb/modeldb"
 	"github.com/contiv/netplugin/utils"
 	"github.com/contiv/netplugin/utils/netutils"
-	"io/ioutil"
-	"net/http"
 
 	log "github.com/Sirupsen/logrus"
 	"github.com/gorilla/mux"
@@ -225,21 +226,35 @@ func (ac *APIController) GlobalUpdate(global, params *contivModel.Global) error
 
 	gCfg := &gstate.Cfg{}
 	gCfg.StateDriver = stateDriver
-	numVlans, vlansInUse := gCfg.GetVlansInUse()
-	numVxlans, vxlansInUse := gCfg.GetVxlansInUse()
+	numVlans, _ := gCfg.GetVlansInUse()
+	numVxlans, _ := gCfg.GetVxlansInUse()
 
 	// Build global config
 	globalCfg := intent.ConfigGlobal{}
 
+	// Generate helpful error message when networks exist
+	errExistingNetworks := func(optionLabel string) error {
+		msgs := []string{}
+		if numVlans > 0 {
+			msgs = append(msgs, fmt.Sprintf("%d vlans", numVlans))
+		}
+		if numVxlans > 0 {
+			msgs = append(msgs, fmt.Sprintf("%d vxlans", numVxlans))
+		}
+		msg := fmt.Sprintf("Unable to update %s due to existing %s",
+			optionLabel, strings.Join(msgs, " and "))
+		log.Errorf(msg)
+		return fmt.Errorf(msg)
+	}
+
 	//check for change in forwarding mode
 	if global.FwdMode != params.FwdMode {
 		//check if there exists any non default network and tenants
 		if numVlans+numVxlans > 0 {
-			log.Errorf("Unable to update forwarding mode due to existing %d vlans and %d vxlans", numVlans, numVxlans)
-			return fmt.Errorf("please delete %v vlans and %v vxlans before changing forwarding mode", vlansInUse, vxlansInUse)
+			return errExistingNetworks("forwarding mode")
 		}
 		if global.FwdMode == "routing" {
-			//check if  any bgp configurations exists.
+			//check if any bgp configurations exists.
 			bgpCfgs := &mastercfg.CfgBgpState{}
 			bgpCfgs.StateDriver = stateDriver
 			cfgs, _ := bgpCfgs.ReadAll()
@@ -264,8 +279,7 @@ func (ac *APIController) GlobalUpdate(global, params *contivModel.Global) error
 	}
 	if global.PvtSubnet != params.PvtSubnet {
 		if (global.PvtSubnet != "" || params.PvtSubnet != defHostPvtNet) && numVlans+numVxlans > 0 {
-			log.Errorf("Unable to update provate subnet due to existing networks")
-			return fmt.Errorf("please delete %v vlans and %v vxlans before changing private subnet", vlansInUse, vxlansInUse)
+			return errExistingNetworks("private subnet")
 		}
 		globalCfg.PvtSubnet = params.PvtSubnet
 	}

netmaster/objApi/objapi_test.go

@@ -24,7 +24,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/contiv/contivmodel"
+	contivModel "github.com/contiv/contivmodel"
 	"github.com/contiv/contivmodel/client"
 	"github.com/contiv/netplugin/core"
 	"github.com/contiv/netplugin/netmaster/gstate"
@@ -297,7 +297,10 @@ func checkInspectGlobal(t *testing.T, expError bool, allocedVlans, allocedVxlans
 }
 
 // checkGlobalSet sets global state and verifies state
-func checkGlobalSet(t *testing.T, expError bool, fabMode, vlans, vxlans, fwdMode, arpMode, pvtSubnet string) {
+// the return error can be used for validating the error produced
+// by contivClient.GlobalPost, a non-nil return is not an error, 'expError'
+// parameter determines if an err for GlobalPost is a failure for the test
+func checkGlobalSet(t *testing.T, expError bool, fabMode, vlans, vxlans, fwdMode, arpMode, pvtSubnet string) error {
 	gl := client.Global{
 		Name:             "global",
 		NetworkInfraType: fabMode,
@@ -358,7 +361,9 @@ func checkGlobalSet(t *testing.T, expError bool, fabMode, vlans, vxlans, fwdMode
 		if err := vlanRsrc.Read("global"); err != nil {
 			t.Fatalf("Error reading vlan resource. Err: %v", err)
 		}
+		return nil
 	}
+	return err
 }
 
 // checkAciGwSet sets AciGw state and verifies verifies it
@@ -1325,8 +1330,80 @@ func TestDynamicGlobalVxlanRange(t *testing.T) {
 
 }
 
+// TestGlobalSetting tests global REST api
+func TestGlobalSettingFwdMode(t *testing.T) {
+	// set to default values (no-op)
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
+
+	// Create a vxlan network to verify you cannot change forward mode
+	checkCreateNetwork(t, false, "default", "contiv7", "", "vxlan", "16.1.1.1/24", "16.1.1.254", 3200, "", "", "")
+
+	// Should fail when changing the forwarding mode whenever there is a network
+	var err error
+	var expErr string
+	err = checkGlobalSet(t, true, "default", "1-4094", "1-10000", "routing", "proxy", "172.19.0.0/16")
+	expErr = "Unable to update forwarding mode due to existing 1 vxlans"
+	if strings.TrimSpace(err.Error()) != expErr {
+		t.Fatalf("Wrong error message, expected: '%v', got '%v'", expErr, err.Error())
+	}
+	// remove the vxlan network and add a vlan network
+	checkDeleteNetwork(t, false, "default", "contiv7")
+	checkCreateNetwork(t, false, "default", "contiv7", "", "vlan", "16.1.1.1/24", "16.1.1.254", 3200, "", "", "")
+	err = checkGlobalSet(t, true, "default", "1-4094", "1-10000", "routing", "proxy", "172.19.0.0/16")
+	expErr = "Unable to update forwarding mode due to existing 1 vlans"
+	if strings.TrimSpace(err.Error()) != expErr {
+		t.Fatalf("Wrong error message, expected: '%v', got '%v'", expErr, err.Error())
+	}
+
+	// remove the vlan network
+	checkDeleteNetwork(t, false, "default", "contiv7")
+
+	// make sure can change forwarding mode after network deleted
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "routing", "proxy", "172.21.0.0/16")
+
+	// reset back to default values
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
+}
+
+func TestGlobalSettingSubnet(t *testing.T) {
+	// set to default values (no-op)
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
+	// Create a network to see if global subnet changes are blocked
+	checkCreateNetwork(t, false, "default", "contiv7", "", "vxlan", "16.1.1.1/24", "16.1.1.254", 3200, "", "", "")
+
+	var err error
+	var expErr string
+	// This should fail
+	err = checkGlobalSet(t, true, "default", "1-4094", "1-10000", "bridge", "proxy", "172.21.0.0/16")
+	expErr = "Unable to update private subnet due to existing 1 vxlans"
+	if strings.TrimSpace(err.Error()) != expErr {
+		t.Fatalf("Wrong error message, expected: '%v', got '%v'", expErr, err.Error())
+	}
+
+	// remove the vxlan network and add a vlan network
+	checkDeleteNetwork(t, false, "default", "contiv7")
+	checkCreateNetwork(t, false, "default", "contiv7", "", "vlan", "16.1.1.1/24", "16.1.1.254", 3200, "", "", "")
+
+	// This should still fail
+	err = checkGlobalSet(t, true, "default", "1-4094", "1-10000", "bridge", "proxy", "172.21.0.0/16")
+	expErr = "Unable to update private subnet due to existing 1 vlans"
+	if strings.TrimSpace(err.Error()) != expErr {
+		t.Fatalf("Wrong error message, expected: '%v', got '%v'", expErr, err.Error())
+	}
+
+	// remove the network
+	checkDeleteNetwork(t, false, "default", "contiv7")
+	// make sure can change subnet after network deleted
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.21.0.0/16")
+	// reset back to default values
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
+}
+
 // TestGlobalSetting tests global REST api
 func TestGlobalSetting(t *testing.T) {
+	// set to default values (no-op)
+	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
+
 	// try basic modification
 	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.20.0.0/16")
 	// set aci mode
@@ -1350,11 +1427,6 @@ func TestGlobalSetting(t *testing.T) {
 	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.21.0.0/16")
 	// Try invalid pvt subnet
 	checkGlobalSet(t, true, "default", "1-4094", "1-10000", "bridge", "proxy", "172.21.0.0/24")
-	// Try changing subnet with active network
-	checkCreateNetwork(t, false, "default", "contiv7", "", "vxlan", "16.1.1.1/24", "16.1.1.254", 3200, "", "", "")
-	checkGlobalSet(t, true, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
-	checkDeleteNetwork(t, false, "default", "contiv7")
-	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")
 
 	// reset back to default values
 	checkGlobalSet(t, false, "default", "1-4094", "1-10000", "bridge", "proxy", "172.19.0.0/16")