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 d828280
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 29 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())
})
})

0 comments on commit d828280

Please sign in to comment.