Skip to content

Commit

Permalink
bridge: clean ip masq if netns is empty
Browse files Browse the repository at this point in the history
In the Del command it didn't clean ip masq when netns is empty.
Add the clean-up code if ip address can fetch from prevResult in StdinData.

Fix #810

Signed-off-by: hyphen.wang <[email protected]>
  • Loading branch information
hyphen.wang authored and qkboy committed Sep 8, 2024
1 parent 20f31e5 commit bbf8920
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 17 deletions.
100 changes: 83 additions & 17 deletions plugins/main/bridge/bridge.go
Original file line number Diff line number Diff line change
Expand Up @@ -774,10 +774,6 @@ func cmdDel(args *skel.CmdArgs) error {
return nil
}

if args.Netns == "" {
return ipamDel()
}

// There is a netns so try to clean up. Delete can be called multiple times
// so don't return an error if the device is already removed.
// If the device isn't there then don't try to clean up IP masq either.
Expand All @@ -791,28 +787,34 @@ func cmdDel(args *skel.CmdArgs) error {
return err
})
if err != nil {
// if NetNs is passed down by the Cloud Orchestration Engine, or if it called multiple times
// if NetNs is passed down by the Cloud Orchestration Engine, or if it called multiple times
// so don't return an error if the device is already removed.
// https://github.com/kubernetes/kubernetes/issues/43014#issuecomment-287164444
_, ok := err.(ns.NSPathNotExistErr)
if ok {
return ipamDel()
if _, isNotExists := err.(ns.NSPathNotExistErr); !isNotExists {
return err
}
return err
}

// call ipam.ExecDel after clean up device in netns
if err := ipamDel(); err != nil {
return err
}
if len(ipnets) == 0 {
// could not get ip address within netns, so try to get it from prevResult
prevResult, err := parsePrevResult(n)
if err != nil {
return err
}

if n.MacSpoofChk {
sc := link.NewSpoofChecker("", "", uniqueID(args.ContainerID, args.IfName))
if err := sc.Teardown(); err != nil {
fmt.Fprintf(os.Stderr, "%v", err)
if prevResult != nil {
ipCfgs, err := getIPCfgs(args.IfName, prevResult)
if err != nil {
return err
}

for _, ip := range ipCfgs {
ipnets = append(ipnets, &ip.Address)
}
}
}

// clean up IP masq first
if isLayer3 && n.IPMasq {
chain := utils.FormatChainName(n.Name, args.ContainerID)
comment := utils.FormatComment(n.Name, args.ContainerID)
Expand All @@ -823,6 +825,18 @@ func cmdDel(args *skel.CmdArgs) error {
}
}

// call ipam.ExecDel after clean up device in netns
if err := ipamDel(); err != nil {
return err
}

if n.MacSpoofChk {
sc := link.NewSpoofChecker("", "", uniqueID(args.ContainerID, args.IfName))
if err := sc.Teardown(); err != nil {
fmt.Fprintf(os.Stderr, "%v", err)
}
}

return err
}

Expand Down Expand Up @@ -1094,3 +1108,55 @@ func cmdCheck(args *skel.CmdArgs) error {
func uniqueID(containerID, cniIface string) string {
return containerID + "-" + cniIface
}

// parsePrevResult parse previous result
func parsePrevResult(conf *NetConf) (*current.Result, error) {
var prevResult *current.Result
if conf.RawPrevResult != nil {
resultBytes, err := json.Marshal(conf.RawPrevResult)
if err != nil {
return nil, fmt.Errorf("could not serialize prevResult: %#v", err)
}
res, err := version.NewResult(conf.CNIVersion, resultBytes)
if err != nil {
return nil, fmt.Errorf("could not parse prevResult: %v", err)
}
conf.RawPrevResult = nil
prevResult, err = current.NewResultFromResult(res)
if err != nil {
return nil, fmt.Errorf("could not convert result to current version: %v", err)
}
}
return prevResult, nil
}

// getIPCfgs finds the IPs on the supplied interface, returning as IPConfig structures
func getIPCfgs(iface string, prevResult *current.Result) ([]*current.IPConfig, error) {
if len(prevResult.IPs) == 0 {
// No IP addresses; that makes no sense. Pack it in.
return nil, fmt.Errorf("No IP addresses supplied on interface: %s", iface)
}

// ips contains the IPConfig structures that were passed, filtered somewhat
ipCfgs := make([]*current.IPConfig, 0, len(prevResult.IPs))

for _, ipCfg := range prevResult.IPs {
// IPs have an interface that is an index into the interfaces array.
// We assume a match if this index is missing.
if ipCfg.Interface == nil {
ipCfgs = append(ipCfgs, ipCfg)
continue
}

// Skip all IPs we know belong to an interface with the wrong name.
intIdx := *ipCfg.Interface
// We do a single interface name, stored in args.IfName
if intIdx >= 0 && intIdx < len(prevResult.Interfaces) && prevResult.Interfaces[intIdx].Name != iface {
continue
}

ipCfgs = append(ipCfgs, ipCfg)
}

return ipCfgs, nil
}
21 changes: 21 additions & 0 deletions plugins/main/bridge/bridge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1740,6 +1740,9 @@ func cmdAddDelCheckTest(origNS, targetNS ns.NetNS, tc testCase, dataDir string)

Expect(prevResult).NotTo(BeNil())

// Check ips config from prevResult
checkPreResult(prevResult)

confString := tc.netConfJSON(dataDir)

conf := &Net{}
Expand All @@ -1766,6 +1769,24 @@ func cmdAddDelCheckTest(origNS, targetNS ns.NetNS, tc testCase, dataDir string)
}
}

func checkPreResult(prevResult types.Result) {
result, err := types100.GetResult(prevResult)
Expect(err).NotTo(HaveOccurred())

ipCfgs, err := getIPCfgs(IFNAME, result)
Expect(err).NotTo(HaveOccurred())

for _, ipCfg := range ipCfgs {
Expect(ipCfg.Gateway).NotTo(BeEmpty())
if ipVersion(ipCfg.Address.IP) == "4" {
Expect(ipCfg.Address.IP.To4()).NotTo(BeNil())
} else {
Expect(ipCfg.Address.IP.To16()).NotTo(BeNil())
}
Expect(ipCfg.Address.Mask).NotTo(BeEmpty())
}
}

var _ = Describe("bridge Operations", func() {
var originalNS, targetNS ns.NetNS
var dataDir string
Expand Down

0 comments on commit bbf8920

Please sign in to comment.