From 47725c63522095dc59ec2a7f6c4a6ef001656fd2 Mon Sep 17 00:00:00 2001 From: "hyphen.wang" Date: Wed, 18 Jan 2023 11:35:31 +0800 Subject: [PATCH] bridge: clean ip masq if netns is empty 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 --- plugins/main/bridge/bridge.go | 100 ++++++++++++++++++++++++----- plugins/main/bridge/bridge_test.go | 21 ++++++ 2 files changed, 104 insertions(+), 17 deletions(-) diff --git a/plugins/main/bridge/bridge.go b/plugins/main/bridge/bridge.go index 4054f6176..e7190a117 100644 --- a/plugins/main/bridge/bridge.go +++ b/plugins/main/bridge/bridge.go @@ -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. @@ -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) @@ -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 } @@ -1088,3 +1102,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 +} diff --git a/plugins/main/bridge/bridge_test.go b/plugins/main/bridge/bridge_test.go index 42c70ae29..748fdfb00 100644 --- a/plugins/main/bridge/bridge_test.go +++ b/plugins/main/bridge/bridge_test.go @@ -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{} @@ -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