Skip to content

bridge: Enable disabling bridge interface #997

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

Merged
squeed merged 1 commit into from
Feb 2, 2024
Merged

bridge: Enable disabling bridge interface #997

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 39 additions & 28 deletions plugins/main/bridge/bridge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,20 @@ 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"`
VlanTrunk []*VlanTrunk `json:"vlanTrunk,omitempty"`
PreserveDefaultVlan bool `json:"preserveDefaultVlan"`
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"`
VlanTrunk []*VlanTrunk `json:"vlanTrunk,omitempty"`
PreserveDefaultVlan bool `json:"preserveDefaultVlan"`
MacSpoofChk bool `json:"macspoofchk,omitempty"`
EnableDad bool `json:"enabledad,omitempty"`
DisableContainerInterface bool `json:"disableContainerInterface,omitempty"`

Args struct {
Cni BridgeArgs `json:"cni,omitempty"`
Expand Down Expand Up @@ -530,6 +531,10 @@ func cmdAdd(args *skel.CmdArgs) error {

isLayer3 := n.IPAM.Type != ""

if isLayer3 && n.DisableContainerInterface {
return fmt.Errorf("cannot use IPAM when DisableContainerInterface flag is set")
}

if n.IsDefaultGW {
n.IsGW = true
}
Expand Down Expand Up @@ -676,12 +681,13 @@ func cmdAdd(args *skel.CmdArgs) error {
}
}
}
} else {
} else if !n.DisableContainerInterface {
if err := netns.Do(func(_ ns.NetNS) error {
link, err := netlink.LinkByName(args.IfName)
if err != nil {
return fmt.Errorf("failed to retrieve link: %v", err)
}

// If layer 2 we still need to set the container veth to up
if err = netlink.LinkSetUp(link); err != nil {
return fmt.Errorf("failed to set %q up: %v", args.IfName, err)
Expand All @@ -692,23 +698,28 @@ func cmdAdd(args *skel.CmdArgs) error {
}
}

var hostVeth netlink.Link
hostVeth, err := netlink.LinkByName(hostInterface.Name)
if err != nil {
return err
}

// check bridge port state
retries := []int{0, 50, 500, 1000, 1000}
for idx, sleep := range retries {
time.Sleep(time.Duration(sleep) * time.Millisecond)
if !n.DisableContainerInterface {
// check bridge port state
retries := []int{0, 50, 500, 1000, 1000}
for idx, sleep := range retries {
time.Sleep(time.Duration(sleep) * time.Millisecond)

hostVeth, err = netlink.LinkByName(hostInterface.Name)
if err != nil {
return err
}
if hostVeth.Attrs().OperState == netlink.OperUp {
break
}
hostVeth, err = netlink.LinkByName(hostInterface.Name)
if err != nil {
return err
}
if hostVeth.Attrs().OperState == netlink.OperUp {
break
}

if idx == len(retries)-1 {
return fmt.Errorf("bridge port in error state: %s", hostVeth.Attrs().OperState)
if idx == len(retries)-1 {
return fmt.Errorf("bridge port in error state: %s", hostVeth.Attrs().OperState)
}
}
}
Copy link
Member

@mlguerrero12 mlguerrero12 Jan 11, 2024

Choose a reason for hiding this comment

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

from line 581 to here, nothing should be run (except hostVeth initialization) when DisableContainerInterface is set, so I think we could encapsulate this in a function to improve readability. We could simply call that function when DisableContainerInterface is false.

Or multiple functions, something like:
if !DisableContainerInterface {
if layer3 {
setupl3()
} else {
l2link up function or code
}

checkBridgePortSTate()
}

WDYT?

Copy link
Contributor Author

@ormergi ormergi Jan 14, 2024

Choose a reason for hiding this comment

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

I have posted a PR for small refactoring around the places this PR change #998
Can we please address what you suggest on a follow up PR?

Copy link
Member

@mlguerrero12 mlguerrero12 Jan 15, 2024

Choose a reason for hiding this comment

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

sure


Expand Down
71 changes: 65 additions & 6 deletions plugins/main/bridge/bridge_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,12 @@ type testCase struct {
removeDefaultVlan bool
ipMasq bool
macspoofchk bool
AddErr020 string
DelErr020 string
AddErr010 string
DelErr010 string
disableContIface bool

AddErr020 string
DelErr020 string
AddErr010 string
DelErr010 string

envArgs string // CNI_ARGS
runtimeConfig struct {
Expand Down Expand Up @@ -154,6 +156,9 @@ const (
netDefault = `,
"isDefaultGateway": true`

disableContainerInterface = `,
"disableContainerInterface": true`

ipamStartStr = `,
"ipam": {
"type": "host-local"`
Expand Down Expand Up @@ -248,6 +253,10 @@ func (tc testCase) netConfJSON(dataDir string) string {
conf += fmt.Sprintf(macspoofchkFormat, tc.macspoofchk)
}

if tc.disableContIface {
conf += disableContainerInterface
}

if !tc.isLayer2 {
conf += netDefault
if tc.subnet != "" || tc.ranges != nil {
Expand Down Expand Up @@ -677,14 +686,16 @@ func (tester *testerV10x) cmdAddTest(tc testCase, dataDir string) (types.Result,
Expect(err).NotTo(HaveOccurred())
Expect(link.Attrs().Name).To(Equal(IFNAME))
Expect(link).To(BeAssignableToTypeOf(&netlink.Veth{}))
assertContainerInterfaceLinkState(&tc, link)

expCIDRsV4, expCIDRsV6 := tc.expectedCIDRs()
addrs, err := netlink.AddrList(link, netlink.FAMILY_V4)
Expect(err).NotTo(HaveOccurred())
Expect(addrs).To(HaveLen(len(expCIDRsV4)))
addrs, err = netlink.AddrList(link, netlink.FAMILY_V6)
Expect(addrs).To(HaveLen(len(expCIDRsV6) + 1)) // add one for the link-local
Expect(err).NotTo(HaveOccurred())
assertIPv6Addresses(&tc, addrs, expCIDRsV6)

// Ignore link local address which may or may not be
// ready when we read addresses.
var foundAddrs int
Expand Down Expand Up @@ -728,6 +739,15 @@ func (tester *testerV10x) cmdAddTest(tc testCase, dataDir string) (types.Result,
return result, nil
}

func assertContainerInterfaceLinkState(tc *testCase, link netlink.Link) {
linkState := int(link.Attrs().OperState)
if tc.disableContIface {
Expect(linkState).ToNot(Equal(netlink.OperUp))
} else {
Expect(linkState).To(Equal(netlink.OperUp))
}
}

func (tester *testerV10x) cmdCheckTest(tc testCase, conf *Net, _ string) {
// Generate network config and command arguments
tester.args = tc.createCheckCmdArgs(tester.targetNS, conf)
Expand Down Expand Up @@ -1008,8 +1028,9 @@ func (tester *testerV04x) cmdAddTest(tc testCase, dataDir string) (types.Result,
Expect(err).NotTo(HaveOccurred())
Expect(addrs).To(HaveLen(len(expCIDRsV4)))
addrs, err = netlink.AddrList(link, netlink.FAMILY_V6)
Expect(addrs).To(HaveLen(len(expCIDRsV6) + 1)) // add one for the link-local
Expect(err).NotTo(HaveOccurred())
assertIPv6Addresses(&tc, addrs, expCIDRsV6)

// Ignore link local address which may or may not be
// ready when we read addresses.
var foundAddrs int
Expand Down Expand Up @@ -1053,6 +1074,14 @@ func (tester *testerV04x) cmdAddTest(tc testCase, dataDir string) (types.Result,
return result, nil
}

func assertIPv6Addresses(tc *testCase, addrs []netlink.Addr, expCIDRsV6 []*net.IPNet) {
if tc.disableContIface {
Expect(addrs).To(BeEmpty())
} else {
Expect(addrs).To(HaveLen(len(expCIDRsV6) + 1)) // add one for the link-local
}
}

func (tester *testerV04x) cmdCheckTest(tc testCase, conf *Net, _ string) {
// Generate network config and command arguments
tester.args = tc.createCheckCmdArgs(tester.targetNS, conf)
Expand Down Expand Up @@ -2461,6 +2490,36 @@ var _ = Describe("bridge Operations", func() {
return nil
})).To(Succeed())
})

It(fmt.Sprintf("[%s] should fail when both IPAM and DisableContainerInterface are set", ver), func() {
Expect(originalNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()
tc := testCase{
cniVersion: ver,
subnet: "10.1.2.0/24",
disableContIface: true,
}
args := tc.createCmdArgs(targetNS, dataDir)
Expect(cmdAdd(args)).To(HaveOccurred())

return nil
})).To(Succeed())
})

It(fmt.Sprintf("[%s] should set the container veth peer state down", ver), func() {
Expect(originalNS.Do(func(ns.NetNS) error {
defer GinkgoRecover()
tc := testCase{
cniVersion: ver,
disableContIface: true,
isLayer2: true,
AddErr020: "cannot convert: no valid IP addresses",
AddErr010: "cannot convert: no valid IP addresses",
}
cmdAddDelTest(originalNS, targetNS, tc, dataDir)
return nil
})).To(Succeed())
})
}

It("check vlan id when loading net conf", func() {
Expand Down