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

Jump to bottom

Add parameter to disable default vlan #875

Merged
merged 1 commit into from
Apr 17, 2023
Merged

Add parameter to disable default vlan #875

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
61 changes: 45 additions & 16 deletions plugins/main/bridge/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,17 +46,18 @@ const defaultBrName = "cni0"

type NetConf struct {
types.NetConf
BrName string `json:"bridge"`
IsGW bool `json:"isGateway"`
IsDefaultGW bool `json:"isDefaultGateway"`
ForceAddress bool `json:"forceAddress"`
IPMasq bool `json:"ipMasq"`
MTU int `json:"mtu"`
HairpinMode bool `json:"hairpinMode"`
PromiscMode bool `json:"promiscMode"`
Vlan int `json:"vlan"`
MacSpoofChk bool `json:"macspoofchk,omitempty"`
EnableDad bool `json:"enabledad,omitempty"`
BrName string `json:"bridge"`
IsGW bool `json:"isGateway"`
IsDefaultGW bool `json:"isDefaultGateway"`
ForceAddress bool `json:"forceAddress"`
IPMasq bool `json:"ipMasq"`
MTU int `json:"mtu"`
HairpinMode bool `json:"hairpinMode"`
PromiscMode bool `json:"promiscMode"`
Vlan int `json:"vlan"`
PreserveDefaultVlan bool `json:"preserveDefaultVlan"`
Copy link

Choose a reason for hiding this comment

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

This will require an update of https://github.com/containernetworking/cni.dev/blob/main/content/plugins/current/main/bridge.md too

Copy link
Member

Choose a reason for hiding this comment

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

Should this be a *bool to preserve the existing behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's another option.

Right now, the default value of this parameter is set to true in line 98 (before unmarshaling the config). So, users have to explicitly set it to false to disable it. If it is not present, the value is set to true.

I didn't want this param to be different than the others, that's why I didn't want to do it with a pointer. Also, because it will need an additional check.

MacSpoofChk bool `json:"macspoofchk,omitempty"`
EnableDad bool `json:"enabledad,omitempty"`

Args struct {
Cni BridgeArgs `json:"cni,omitempty"`
Expand Down Expand Up @@ -94,6 +95,8 @@ func init() {
func loadNetConf(bytes []byte, envArgs string) (*NetConf, string, error) {
n := &NetConf{
BrName: defaultBrName,
// Set default value equal to true to maintain existing behavior.
PreserveDefaultVlan: true,
}
mlguerrero12 marked this conversation as resolved.
Show resolved Hide resolved
if err := json.Unmarshal(bytes, n); err != nil {
return nil, "", fmt.Errorf("failed to load netconf: %v", err)
Expand Down Expand Up @@ -299,7 +302,7 @@ func ensureBridge(brName string, mtu int, promiscMode, vlanFiltering bool) (*net
return br, nil
}

func ensureVlanInterface(br *netlink.Bridge, vlanID int) (netlink.Link, error) {
func ensureVlanInterface(br *netlink.Bridge, vlanID int, preserveDefaultVlan bool) (netlink.Link, error) {
name := fmt.Sprintf("%s.%d", br.Name, vlanID)

brGatewayVeth, err := netlink.LinkByName(name)
Expand All @@ -313,7 +316,7 @@ func ensureVlanInterface(br *netlink.Bridge, vlanID int) (netlink.Link, error) {
return nil, fmt.Errorf("faild to find host namespace: %v", err)
}

_, brGatewayIface, err := setupVeth(hostNS, br, name, br.MTU, false, vlanID, "")
_, brGatewayIface, err := setupVeth(hostNS, br, name, br.MTU, false, vlanID, preserveDefaultVlan, "")
if err != nil {
return nil, fmt.Errorf("faild to create vlan gateway %q: %v", name, err)
}
Expand All @@ -332,7 +335,7 @@ func ensureVlanInterface(br *netlink.Bridge, vlanID int) (netlink.Link, error) {
return brGatewayVeth, nil
}

func setupVeth(netns ns.NetNS, br *netlink.Bridge, ifName string, mtu int, hairpinMode bool, vlanID int, mac string) (*current.Interface, *current.Interface, error) {
func setupVeth(netns ns.NetNS, br *netlink.Bridge, ifName string, mtu int, hairpinMode bool, vlanID int, preserveDefaultVlan bool, mac string) (*current.Interface, *current.Interface, error) {
contIface := &current.Interface{}
hostIface := &current.Interface{}

Expand Down Expand Up @@ -370,6 +373,13 @@ func setupVeth(netns ns.NetNS, br *netlink.Bridge, ifName string, mtu int, hairp
}

if vlanID != 0 {
if !preserveDefaultVlan {
mlguerrero12 marked this conversation as resolved.
Show resolved Hide resolved
err = removeDefaultVlan(hostVeth)
if err != nil {
return nil, nil, fmt.Errorf("failed to remove default vlan on interface %q: %v", hostIface.Name, err)
}
}

err = netlink.BridgeVlanAdd(hostVeth, uint16(vlanID), true, true, false, true)
if err != nil {
return nil, nil, fmt.Errorf("failed to setup vlan tag on interface %q: %v", hostIface.Name, err)
Expand All @@ -379,6 +389,25 @@ func setupVeth(netns ns.NetNS, br *netlink.Bridge, ifName string, mtu int, hairp
return hostIface, contIface, nil
}

func removeDefaultVlan(hostVeth netlink.Link) error {
vlanInfo, err := netlink.BridgeVlanList()
if err != nil {
return err
}

brVlanInfo, ok := vlanInfo[int32(hostVeth.Attrs().Index)]
if ok {
for _, info := range brVlanInfo {
err = netlink.BridgeVlanDel(hostVeth, info.Vid, false, false, false, true)
if err != nil {
return err
}
}
}

return nil
}

func calcGatewayIP(ipn *net.IPNet) net.IP {
nid := ipn.IP.Mask(ipn.Mask)
return ip.NextIP(nid)
Expand Down Expand Up @@ -434,7 +463,7 @@ func cmdAdd(args *skel.CmdArgs) error {
}
defer netns.Close()

hostInterface, containerInterface, err := setupVeth(netns, br, args.IfName, n.MTU, n.HairpinMode, n.Vlan, n.mac)
hostInterface, containerInterface, err := setupVeth(netns, br, args.IfName, n.MTU, n.HairpinMode, n.Vlan, n.PreserveDefaultVlan, n.mac)
if err != nil {
return err
}
Expand Down Expand Up @@ -523,7 +552,7 @@ func cmdAdd(args *skel.CmdArgs) error {
firstV4Addr = gw.IP
}
if n.Vlan != 0 {
vlanIface, err := ensureVlanInterface(br, n.Vlan)
vlanIface, err := ensureVlanInterface(br, n.Vlan, n.PreserveDefaultVlan)
if err != nil {
return fmt.Errorf("failed to create vlan interface: %v", err)
}
Expand Down
79 changes: 64 additions & 15 deletions plugins/main/bridge/bridge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,21 +65,22 @@ type Net struct {
// testCase defines the CNI network configuration and the expected
// bridge addresses for a test case.
type testCase struct {
cniVersion string // CNI Version
subnet string // Single subnet config: Subnet CIDR
gateway string // Single subnet config: Gateway
ranges []rangeInfo // Ranges list (multiple subnets config)
resolvConf string // host-local resolvConf file path
isGW bool
isLayer2 bool
expGWCIDRs []string // Expected gateway addresses in CIDR form
vlan int
ipMasq bool
macspoofchk bool
AddErr020 string
DelErr020 string
AddErr010 string
DelErr010 string
cniVersion string // CNI Version
subnet string // Single subnet config: Subnet CIDR
gateway string // Single subnet config: Gateway
ranges []rangeInfo // Ranges list (multiple subnets config)
resolvConf string // host-local resolvConf file path
isGW bool
isLayer2 bool
expGWCIDRs []string // Expected gateway addresses in CIDR form
vlan int
removeDefaultVlan bool
mlguerrero12 marked this conversation as resolved.
Show resolved Hide resolved
ipMasq bool
macspoofchk bool
AddErr020 string
DelErr020 string
AddErr010 string
DelErr010 string

envArgs string // CNI_ARGS
runtimeConfig struct {
Expand Down Expand Up @@ -129,6 +130,9 @@ const (
vlan = `,
"vlan": %d`

preserveDefaultVlan = `,
"preserveDefaultVlan": false`

netDefault = `,
"isDefaultGateway": true`

Expand Down Expand Up @@ -191,6 +195,10 @@ func (tc testCase) netConfJSON(dataDir string) string {
conf := fmt.Sprintf(netConfStr, tc.cniVersion, BRNAME)
if tc.vlan != 0 {
conf += fmt.Sprintf(vlan, tc.vlan)

if tc.removeDefaultVlan {
conf += preserveDefaultVlan
}
}
if tc.ipMasq {
conf += tc.ipMasqConfig()
Expand Down Expand Up @@ -527,6 +535,9 @@ func (tester *testerV10x) cmdAddTest(tc testCase, dataDir string) (types.Result,
vlans, isExist := interfaceMap[int32(peerLink.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check the bridge vlan filtering equals true
Expand Down Expand Up @@ -582,6 +593,9 @@ func (tester *testerV10x) cmdAddTest(tc testCase, dataDir string) (types.Result,
vlans, isExist := interfaceMap[int32(link.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check that the bridge has a different mac from the veth
Expand Down Expand Up @@ -832,6 +846,9 @@ func (tester *testerV04x) cmdAddTest(tc testCase, dataDir string) (types.Result,
vlans, isExist := interfaceMap[int32(peerLink.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check the bridge vlan filtering equals true
Expand Down Expand Up @@ -887,6 +904,9 @@ func (tester *testerV04x) cmdAddTest(tc testCase, dataDir string) (types.Result,
vlans, isExist := interfaceMap[int32(link.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check that the bridge has a different mac from the veth
Expand Down Expand Up @@ -1132,6 +1152,9 @@ func (tester *testerV03x) cmdAddTest(tc testCase, dataDir string) (types.Result,
vlans, isExist := interfaceMap[int32(peerLink.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check the bridge vlan filtering equals true
Expand Down Expand Up @@ -1187,6 +1210,9 @@ func (tester *testerV03x) cmdAddTest(tc testCase, dataDir string) (types.Result,
vlans, isExist := interfaceMap[int32(link.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check that the bridge has a different mac from the veth
Expand Down Expand Up @@ -1358,6 +1384,9 @@ func (tester *testerV01xOr02x) cmdAddTest(tc testCase, dataDir string) (types.Re
vlans, isExist := interfaceMap[int32(peerLink.Attrs().Index)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

// Check the bridge vlan filtering equals true
Expand Down Expand Up @@ -1480,6 +1509,9 @@ func (tester *testerV01xOr02x) cmdAddTest(tc testCase, dataDir string) (types.Re
vlans, isExist := hostNSVlanMap[int32(peerIndex)]
Expect(isExist).To(BeTrue())
Expect(checkVlan(tc.vlan, vlans)).To(BeTrue())
if tc.removeDefaultVlan {
Expect(vlans).To(HaveLen(1))
}
}

return nil
Expand Down Expand Up @@ -1772,6 +1804,18 @@ var _ = Describe("bridge Operations", func() {
cmdAddDelTest(originalNS, targetNS, tc, dataDir)
})

It(fmt.Sprintf("[%s] configures and deconfigures a l2 bridge with vlan id 100 and no default vlan using ADD/DEL", ver), func() {
tc := testCase{
cniVersion: ver,
isLayer2: true,
vlan: 100,
removeDefaultVlan: true,
AddErr020: "cannot convert: no valid IP addresses",
AddErr010: "cannot convert: no valid IP addresses",
}
cmdAddDelTest(originalNS, targetNS, tc, dataDir)
})

for i, tc := range []testCase{
{
// IPv4 only
Expand Down Expand Up @@ -1822,6 +1866,11 @@ var _ = Describe("bridge Operations", func() {
tc.cniVersion = ver
cmdAddDelTest(originalNS, targetNS, tc, dataDir)
})
It(fmt.Sprintf("[%s] (%d) configures and deconfigures a bridge, veth with default route and vlanID 100 and no default vlan with ADD/DEL", ver, i), func() {
tc.cniVersion = ver
tc.removeDefaultVlan = true
cmdAddDelTest(originalNS, targetNS, tc, dataDir)
})
}

for i, tc := range []testCase{
Expand Down