Skip to content

Commit

Permalink
Call GC command with valid attachments from multus cache
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@s1061123
s1061123 committed May 24, 2024
1 parent 64e5cda commit 4064866
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 31 deletions.
53 changes: 46 additions & 7 deletions pkg/multus/multus.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
if err != nil {
return nil, err
}

allAttachments := []cnitypes.GCAttachment{}
for _, dirEnt := range dirEntries {
path := filepath.Join(cacheDir, dirEnt.Name())
delegatesBytes, err := os.ReadFile(path)

Check failure

Code scanning / CodeQL

Uncontrolled data used in path expression High

This path depends on a
user-provided value
Loading
.
// 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 {
Expand Down Expand Up @@ -1009,8 +1043,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)
Expand Down
14 changes: 0 additions & 14 deletions pkg/multus/multus_cni020_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
43 changes: 35 additions & 8 deletions pkg/multus/multus_cni100_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"os"
"path/filepath"
"reflect"
"sync"
"time"
Expand Down Expand Up @@ -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",
Expand All @@ -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())
})
})
4 changes: 2 additions & 2 deletions pkg/server/api/shim.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down

0 comments on commit 4064866

Please sign in to comment.