Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

global fwd and subnet error msgs fix & test cases #1051

Merged
merged 4 commits into from
Nov 21, 2017
Merged

global fwd and subnet error msgs fix & test cases #1051

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3bf1948
global fwd and subnet error msgs fix & test cases
chrisplo Nov 2, 2017
131b687
simplified
chrisplo Nov 7, 2017
d482c90
err no longer a pointer to error
chrisplo Nov 9, 2017
5e4519f
Merge remote-tracking branch 'origin/master' into fix_nm_output_msg
chrisplo Nov 21, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions netmaster/objApi/apiController.go
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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()
Expand All @@ -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
}
Expand Down
83 changes: 76 additions & 7 deletions netmaster/objApi/objapi_test.go
100755 → 100644
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -297,7 +297,7 @@ 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) {
func checkGlobalSet(t *testing.T, expError bool, fabMode, vlans, vxlans, fwdMode, arpMode, pvtSubnet string) *error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return error instead of *error here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

gl := client.Global{
Name: "global",
NetworkInfraType: fabMode,
Expand Down Expand Up @@ -358,7 +358,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
Expand Down Expand Up @@ -1317,8 +1319,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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this returns an error, we should probably be asserting that it's nil or some expected value...

If there's a legit reason for skipping error checks on a call, maybe add a comment above the call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, i was just copy pasting that part, but I can make a helper checkGlobalSetNotNil(..) to avoid if block every call


// 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
Expand All @@ -1342,11 +1416,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")
Expand Down