From 7eb9673a1ae4e3e6b7e47951646f5c57513d696f Mon Sep 17 00:00:00 2001 From: Tomofumi Hayashi Date: Fri, 24 May 2024 21:43:53 +0900 Subject: [PATCH] Call GC command with valid attachments from multus cache This code changes CNI's GC command argument. Previously it just passes from parent CNI runtime, however, it may causes unexpected resource deletion if one CNI plugin is used in both cluster network and net-attach-def. This change generates valid attachments from multus CNI cache and passed to delegate CNI plugin. --- pkg/multus/multus.go | 53 +++++++++++++++++++++++++++----- pkg/multus/multus_cni020_test.go | 14 --------- pkg/multus/multus_cni100_test.go | 43 +++++++++++++++++++++----- pkg/server/api/shim.go | 4 +-- 4 files changed, 83 insertions(+), 31 deletions(-) diff --git a/pkg/multus/multus.go b/pkg/multus/multus.go index 6e99c5e9a..e42287695 100644 --- a/pkg/multus/multus.go +++ b/pkg/multus/multus.go @@ -131,15 +131,49 @@ func saveDelegates(containerID, dataDir string, delegates []*types.DelegateNetCo return err } -func deleteDelegates(containerID, dataDir string) error { - logging.Debugf("deleteDelegates: %s, %s", containerID, dataDir) +func getValidAttachmentFromCache(b []byte) (string, string, error) { + type simpleCacheV1 struct { + Kind string `json:"kind"` + ContainerID string `json:"containerId"` + IfName string `json:"ifName"` + } - path := filepath.Join(dataDir, containerID) - if err := os.Remove(path); err != nil { - return logging.Errorf("deleteDelegates: error in deleting the delegates : %v", err) + cache := &simpleCacheV1{} + if err := json.Unmarshal(b, cache); err != nil { + return "", "", fmt.Errorf("getValidAttachmentFromCache: invalid json: %v", err) } - return nil + if cache.ContainerID == "" || cache.IfName == "" { + return "", "", fmt.Errorf("invalid cache: containerID:%q, ifName:%q", cache.ContainerID, cache.IfName) + } + + return cache.ContainerID, cache.IfName, nil +} + +func gatherValidAttachmentsFromCache(cniDir string) ([]cnitypes.GCAttachment, error) { + cacheDir := filepath.Join(cniDir, "results") + dirEntries, err := os.ReadDir(cacheDir) + if err != nil { + return nil, err + } + + allAttachments := []cnitypes.GCAttachment{} + for _, dirEnt := range dirEntries { + path := filepath.Join(cacheDir, dirEnt.Name()) + delegatesBytes, err := os.ReadFile(path) + // if delegates cannot read that, skipped for now (because cannot recover). + if err != nil { + logging.Errorf("gatherSavedDelegates: cannot read %q, skipped to add", path) + continue + } + containerID, ifName, err := getValidAttachmentFromCache(delegatesBytes) + if err != nil { + logging.Errorf("gatherSavedDelegates: cannot read cache, skipped to add: %v", err) + continue + } + allAttachments = append(allAttachments, cnitypes.GCAttachment{ContainerID: containerID, IfName: ifName}) + } + return allAttachments, nil } func validateIfName(nsname string, ifname string) error { @@ -1017,8 +1051,13 @@ func CmdGC(args *skel.CmdArgs, exec invoke.Exec, kubeClient *k8s.ClientInfo) err return logging.Errorf("error in converting the raw bytes to conf: %v", err) } + validAttachments, err := gatherValidAttachmentsFromCache(n.CNIDir) + if err != nil { + return logging.Errorf("error in gather valid attachments: %v", err) + } + err = cniNet.GCNetworkList(context.TODO(), conf, &libcni.GCArgs{ - ValidAttachments: n.ValidAttachments, + ValidAttachments: validAttachments, }) if err != nil { return logging.Errorf("error in GC command: %v", err) diff --git a/pkg/multus/multus_cni020_test.go b/pkg/multus/multus_cni020_test.go index 5a1f271f5..b6863a610 100644 --- a/pkg/multus/multus_cni020_test.go +++ b/pkg/multus/multus_cni020_test.go @@ -43,20 +43,6 @@ var _ = Describe("multus operations", func() { err := saveScratchNetConf("123456789", "", meme) Expect(err).To(HaveOccurred()) }) - - It("fails to delete delegates with bad filepath", func() { - err := deleteDelegates("123456789", "bad!file!~?Path$^") - Expect(err).To(HaveOccurred()) - }) - - It("delete delegates given good filepath", func() { - os.MkdirAll("/opt/cni/bin", 0755) - d1 := []byte("blah") - os.WriteFile("/opt/cni/bin/123456789", d1, 0644) - - err := deleteDelegates("123456789", "/opt/cni/bin") - Expect(err).NotTo(HaveOccurred()) - }) }) var _ = Describe("multus operations cniVersion 0.2.0 config", func() { diff --git a/pkg/multus/multus_cni100_test.go b/pkg/multus/multus_cni100_test.go index 633d72a32..cc809170c 100644 --- a/pkg/multus/multus_cni100_test.go +++ b/pkg/multus/multus_cni100_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "os" + "path/filepath" "reflect" "sync" "time" @@ -1309,15 +1310,30 @@ var _ = Describe("multus operations cniVersion 1.1.0 config", func() { }) It("executes delegates with CNI GC", func() { + tmpCNIDir := tmpDir + "/cniData" + err := os.Mkdir(tmpCNIDir, 0777) + Expect(err).NotTo(HaveOccurred()) + + cniCacheDir := filepath.Join(tmpCNIDir, "/results") + err = os.Mkdir(cniCacheDir, 0777) + Expect(err).NotTo(HaveOccurred()) + + //create fake cniResult file + err = os.WriteFile(filepath.Join(cniCacheDir, "cbr0-3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755-eth0"), []byte(`{"kind":"cniCacheV1","containerId":"3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755","config":"eyJjbmlWZXJzaW9uIjoiMC4zLjEiLCJuYW1lIjoiY2JyMCIsInBsdWdpbnMiOlt7ImNhcGFiaWxpdGllcyI6eyJpby5rdWJlcm5ldGVzLmNyaS5wb2QtYW5ub3RhdGlvbnMiOnRydWV9LCJkZWxlZ2F0ZSI6eyJoYWlycGluTW9kZSI6dHJ1ZSwiaXNEZWZhdWx0R2F0ZXdheSI6dHJ1ZX0sInR5cGUiOiJmbGFubmVsIn0seyJjYXBhYmlsaXRpZXMiOnsicG9ydE1hcHBpbmdzIjp0cnVlfSwidHlwZSI6InBvcnRtYXAifV19","ifName":"eth0","networkName":"cbr0","netns":"/var/run/netns/8b8677c8-8929-4746-8206-514069760f6e","cniArgs":[["IgnoreUnknown","true"],["K8S_POD_NAMESPACE","default"],["K8S_POD_NAME","macvlan"],["K8S_POD_INFRA_CONTAINER_ID","3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755"],["K8S_POD_UID","f0bfbd5b-096d-48ef-998c-da26743dd0cb"],["IgnoreUnknown","1"],["K8S_POD_NAMESPACE","default"],["K8S_POD_NAME","macvlan"],["K8S_POD_INFRA_CONTAINER_ID","3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755"],["K8S_POD_UID","f0bfbd5b-096d-48ef-998c-da26743dd0cb"]],"result":{"cniVersion":"0.3.1","dns":{},"interfaces":[{"mac":"ea:19:25:a2:a1:93","name":"cni0"},{"mac":"ba:76:61:2f:8b:ca","name":"vethc42d3d18"},{"mac":"7e:57:6a:9b:6b:b5","name":"eth0","sandbox":"/var/run/netns/8b8677c8-8929-4746-8206-514069760f6e"}],"ips":[{"address":"10.244.1.4/24","gateway":"10.244.1.1","interface":2,"version":"4"}],"routes":[{"dst":"10.244.0.0/16"},{"dst":"0.0.0.0/0","gw":"10.244.1.1"}]}}`), 0666) + Expect(err).NotTo(HaveOccurred()) + err = os.WriteFile(filepath.Join(cniCacheDir, "macvlan-conf-1-3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755-net1"), []byte(`{"kind":"cniCacheV1","containerId":"3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755","config":"eyJjbmlWZXJzaW9uIjoiMC4zLjEiLCJpcGFtIjp7ImFkZHJlc3NlcyI6W3siYWRkcmVzcyI6IjEwLjEuMS4xMDEvMjQifV0sInR5cGUiOiJzdGF0aWMifSwibWFzdGVyIjoiZXRoMSIsIm1vZGUiOiJicmlkZ2UiLCJuYW1lIjoibWFjdmxhbi1jb25mLTEiLCJ0eXBlIjoibWFjdmxhbiJ9","ifName":"net1","networkName":"macvlan-conf-1","netns":"/var/run/netns/8b8677c8-8929-4746-8206-514069760f6e","cniArgs":[["IgnoreUnknown","true"],["K8S_POD_NAMESPACE","default"],["K8S_POD_NAME","macvlan"],["K8S_POD_INFRA_CONTAINER_ID","3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755"],["K8S_POD_UID","f0bfbd5b-096d-48ef-998c-da26743dd0cb"],["IgnoreUnknown","1"],["K8S_POD_NAMESPACE","default"],["K8S_POD_NAME","macvlan"],["K8S_POD_INFRA_CONTAINER_ID","3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755"],["K8S_POD_UID","f0bfbd5b-096d-48ef-998c-da26743dd0cb"]],"result":{"cniVersion":"0.3.1","dns":{},"interfaces":[{"mac":"36:b3:c5:29:ad:b8","name":"net1","sandbox":"/var/run/netns/8b8677c8-8929-4746-8206-514069760f6e"}],"ips":[{"address":"10.1.1.101/24","interface":0,"version":"4"}]}}`), 0666) + Expect(err).NotTo(HaveOccurred()) + args := &skel.CmdArgs{ ContainerID: "123456789", Netns: testNS.Path(), IfName: "eth0", - StdinData: []byte(`{ + StdinData: []byte(fmt.Sprintf(`{ "name": "node-cni-network", "type": "multus", "defaultnetworkfile": "/tmp/foo.multus.conf", "defaultnetworkwaitseconds": 3, + "cniDir": "%s", "delegates": [{ "name": "weave1", "cniVersion": "1.1.0", @@ -1331,23 +1347,34 @@ var _ = Describe("multus operations cniVersion 1.1.0 config", func() { "type": "other-plugin" }] }] - }`), + }`, tmpCNIDir)), } logging.SetLogLevel("verbose") fExec := newFakeExec() expectedConf1 := `{ - "cni.dev/valid-attachments": null, - "name": "weave1", - "cniVersion": "1.1.0", - "type": "weave-net" - }` + "cni.dev/valid-attachments": [ + { + "containerID": "3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755", + "ifname": "eth0" + }, + { + "containerID": "3f6940ab5ab43bc522569d15b23f8c1bbde1d7678b080398506924fc01d72755", + "ifname": "net1" + } + ], + "name": "weave1", + "cniVersion": "1.1.0", + "type": "weave-net" + }` fExec.addPlugin100(nil, "", expectedConf1, nil, nil) - err := CmdGC(args, fExec, nil) + err = CmdGC(args, fExec, nil) Expect(err).NotTo(HaveOccurred()) // we only execute once for cluster network, not additional one Expect(fExec.gcIndex).To(Equal(1)) + err = os.RemoveAll(tmpCNIDir) + Expect(err).NotTo(HaveOccurred()) }) }) diff --git a/pkg/server/api/shim.go b/pkg/server/api/shim.go index 1674ae4fb..6f27763bc 100644 --- a/pkg/server/api/shim.go +++ b/pkg/server/api/shim.go @@ -76,7 +76,7 @@ func CmdDel(args *skel.CmdArgs) error { // CmdGC implements the CNI spec GC command handler func CmdGC(args *skel.CmdArgs) error { - _, _, err := postRequest(args) + _, _, err := postRequest(args, WaitUntilAPIReady) if err != nil { return logging.Errorf("CmdGC (shim): %v", err) } @@ -85,7 +85,7 @@ func CmdGC(args *skel.CmdArgs) error { // CmdStatus implements the CNI spec STATUS command handler func CmdStatus(args *skel.CmdArgs) error { - _, _, err := postRequest(args) + _, _, err := postRequest(args, WaitUntilAPIReady) if err != nil { return logging.Errorf("CmdStatus (shim): %v", err) }