From 17436dc5d41f0d5422611953a370528e7b2e8ba7 Mon Sep 17 00:00:00 2001 From: Henry Wang Date: Sun, 1 Oct 2023 00:38:33 +0000 Subject: [PATCH] resume and complete gc impl Signed-off-by: Henry Wang --- libcni/api.go | 31 +++++------- libcni/api_test.go | 65 ++++++++++++++++++++++-- libcni/multierror.go | 58 ++++++++++++++++++++++ pkg/skel/skel.go | 84 +++++++++++++++++++++++++------- pkg/skel/skel_test.go | 12 +++-- plugins/test/noop/debug/debug.go | 43 +++++++++++----- plugins/test/noop/main.go | 20 +++++++- 7 files changed, 254 insertions(+), 59 deletions(-) create mode 100644 libcni/multierror.go diff --git a/libcni/api.go b/libcni/api.go index 6960a8ab..803c882a 100644 --- a/libcni/api.go +++ b/libcni/api.go @@ -23,7 +23,6 @@ package libcni import ( "context" "encoding/json" - "errors" "fmt" "os" "path/filepath" @@ -114,7 +113,7 @@ type CNI interface { ValidateNetworkList(ctx context.Context, net *NetworkConfigList) ([]string, error) ValidateNetwork(ctx context.Context, net *NetworkConfig) ([]string, error) - GCNetworkList(ctx context.Context, net *NetworkConfigList, args GCArgs) error + GCNetworkList(ctx context.Context, net *NetworkConfigList, args *GCArgs) error GetCachedAttachments(containerID string) ([]*NetworkAttachment, error) } @@ -758,7 +757,7 @@ func (c *CNIConfig) GetVersionInfo(ctx context.Context, pluginType string) (vers // GCNetworkList will do two things // - dump the list of cached attachments, and issue deletes as necessary // - issue a GC to the underlying plugins (if the version is high enough) -func (c *CNIConfig) GCNetworkList(ctx context.Context, list *NetworkConfigList, args GCArgs) error { +func (c *CNIConfig) GCNetworkList(ctx context.Context, list *NetworkConfigList, args *GCArgs) error { // First, get the list of cached attachments cachedAttachments, err := c.GetCachedAttachments("") if err != nil { @@ -770,19 +769,21 @@ func (c *CNIConfig) GCNetworkList(ctx context.Context, list *NetworkConfigList, validAttachments[a] = nil } - errs := []error{} + var errs []error for _, cachedAttachment := range cachedAttachments { if cachedAttachment.Network != list.Name { continue } // we found this attachment - if _, ok := validAttachments[GCAttachment{ContainerID: cachedAttachment.ContainerID, IfName: cachedAttachment.IfName}]; ok { + gca := GCAttachment{ + ContainerID: cachedAttachment.ContainerID, + IfName: cachedAttachment.IfName, + } + if _, ok := validAttachments[gca]; ok { continue } - - // otherwise, this attachment wasn't valid - // and we should issue a CNI DEL + // otherwise, this attachment wasn't valid and we should issue a CNI DEL rt := RuntimeConf{ ContainerID: cachedAttachment.ContainerID, NetNS: cachedAttachment.NetNS, @@ -790,8 +791,7 @@ func (c *CNIConfig) GCNetworkList(ctx context.Context, list *NetworkConfigList, Args: cachedAttachment.CniArgs, CapabilityArgs: cachedAttachment.CapabilityArgs, } - err := c.DelNetworkList(ctx, list, &rt) - if err != nil { + if err := c.DelNetworkList(ctx, list, &rt); err != nil { errs = append(errs, fmt.Errorf("failed to delete stale attachment %s %s: %w", rt.ContainerID, rt.IfName, err)) } } @@ -807,20 +807,15 @@ func (c *CNIConfig) GCNetworkList(ctx context.Context, list *NetworkConfigList, // build config here pluginConfig, err := InjectConf(plugin, inject) if err != nil { - errs = append(errs, fmt.Errorf( - "failed to generate configuration to GC plugin %s: %w", - plugin.Network.Type, err)) + errs = append(errs, fmt.Errorf("failed to generate configuration to GC plugin %s: %w", plugin.Network.Type, err)) } if err := c.gcNetwork(ctx, pluginConfig); err != nil { - errs = append(errs, fmt.Errorf( - "failed to GC plugin %s: %w", - plugin.Network.Type, err)) + errs = append(errs, fmt.Errorf("failed to GC plugin %s: %w", plugin.Network.Type, err)) } } - } - return errors.Join(errs...) + return joinErrors(errs...) } func (c *CNIConfig) gcNetwork(ctx context.Context, net *NetworkConfig) error { diff --git a/libcni/api_test.go b/libcni/api_test.go index 445c6246..1f552325 100644 --- a/libcni/api_test.go +++ b/libcni/api_test.go @@ -1513,16 +1513,71 @@ var _ = Describe("Invoking plugins", func() { _, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig) Expect(err).NotTo(HaveOccurred()) + By("Issuing a GC with valid networks") + gcargs := &libcni.GCArgs{ + ValidAttachments: []libcni.GCAttachment{{ + ContainerID: runtimeConfig.ContainerID, + IfName: runtimeConfig.IfName, + }}, + } + err = cniConfig.GCNetworkList(ctx, netConfigList, gcargs) + Expect(err).NotTo(HaveOccurred()) + By("Issuing a GC with no valid networks") - err = cniConfig.GCNetworkList(ctx, netConfigList, libcni.GCArgs{ - ValidAttachments: []libcni.GCAttachment{}, - }) + gcargs.ValidAttachments = nil + err = cniConfig.GCNetworkList(ctx, netConfigList, gcargs) Expect(err).NotTo(HaveOccurred()) commands, err := noop_debug.ReadCommandLog(plugins[0].commandFilePath) Expect(err).NotTo(HaveOccurred()) - Expect(commands).To(Equal([]string{"ADD", "DEL", "GC"})) - + Expect(commands).To(HaveLen(4)) + + validations := []struct { + name string + fn func(entry noop_debug.CmdLogEntry) + }{ + { + name: "ADD", + fn: func(entry noop_debug.CmdLogEntry) { + Expect(entry.CmdArgs.ContainerID).To(Equal(runtimeConfig.ContainerID)) + Expect(entry.CmdArgs.IfName).To(Equal(runtimeConfig.IfName)) + }, + }, + { + name: "GC", + fn: func(entry noop_debug.CmdLogEntry) { + var conf struct { + Attachments []map[string]string `json:"cni.dev/valid-attachments"` + } + err = json.Unmarshal(entry.CmdArgs.StdinData, &conf) + Expect(err).NotTo(HaveOccurred()) + Expect(conf.Attachments).To(HaveLen(1)) + Expect(conf.Attachments[0]).To(Equal(map[string]string{"containerID": runtimeConfig.ContainerID, "ifname": runtimeConfig.IfName})) + }, + }, + { + name: "DEL", + fn: func(entry noop_debug.CmdLogEntry) { + Expect(entry.CmdArgs.ContainerID).To(Equal(runtimeConfig.ContainerID)) + Expect(entry.CmdArgs.IfName).To(Equal(runtimeConfig.IfName)) + }, + }, + { + name: "GC", + fn: func(entry noop_debug.CmdLogEntry) { + var conf struct { + Attachments []map[string]string `json:"cni.dev/valid-attachments"` + } + err = json.Unmarshal(entry.CmdArgs.StdinData, &conf) + Expect(err).NotTo(HaveOccurred()) + Expect(conf.Attachments).To(BeEmpty()) + }, + }, + } + for i, c := range validations { + Expect(commands[i].Command).To(Equal(c.name)) + c.fn(commands[i]) + } }) }) }) diff --git a/libcni/multierror.go b/libcni/multierror.go new file mode 100644 index 00000000..100fb839 --- /dev/null +++ b/libcni/multierror.go @@ -0,0 +1,58 @@ +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +// Copyright the CNI authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +// Adapted from errors/join.go from go 1.20 +// This package can be removed once the toolchain is updated to 1.20 + +package libcni + +func joinErrors(errs ...error) error { + n := 0 + for _, err := range errs { + if err != nil { + n++ + } + } + if n == 0 { + return nil + } + e := &multiError{ + errs: make([]error, 0, n), + } + for _, err := range errs { + if err != nil { + e.errs = append(e.errs, err) + } + } + return e +} + +type multiError struct { + errs []error +} + +func (e *multiError) Error() string { + var b []byte + for i, err := range e.errs { + if i > 0 { + b = append(b, '\n') + } + b = append(b, err.Error()...) + } + return string(b) +} diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go index 7f5fb8d7..432923dc 100644 --- a/pkg/skel/skel.go +++ b/pkg/skel/skel.go @@ -60,9 +60,10 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { var cmd, contID, netns, ifName, args, path, netnsOverride string vars := []struct { - name string - val *string - reqForCmd reqForCmdEntry + name string + val *string + reqForCmd reqForCmdEntry + validateFn func(string) *types.Error }{ { "CNI_COMMAND", @@ -71,7 +72,9 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "ADD": true, "CHECK": true, "DEL": true, + "GC": true, }, + nil, }, { "CNI_CONTAINERID", @@ -81,6 +84,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "CHECK": true, "DEL": true, }, + utils.ValidateContainerID, }, { "CNI_NETNS", @@ -90,6 +94,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "CHECK": true, "DEL": false, }, + nil, }, { "CNI_IFNAME", @@ -99,6 +104,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "CHECK": true, "DEL": true, }, + utils.ValidateInterfaceName, }, { "CNI_ARGS", @@ -108,6 +114,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "CHECK": false, "DEL": false, }, + nil, }, { "CNI_PATH", @@ -116,7 +123,9 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "ADD": true, "CHECK": true, "DEL": true, + "GC": true, }, + nil, }, { "CNI_NETNS_OVERRIDE", @@ -126,6 +135,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { "CHECK": false, "DEL": false, }, + nil, }, } @@ -136,6 +146,10 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { if v.reqForCmd[cmd] || v.name == "CNI_COMMAND" { argsMissing = append(argsMissing, v.name) } + } else if v.reqForCmd[cmd] && v.validateFn != nil { + if err := v.validateFn(*v.val); err != nil { + return "", nil, err + } } } @@ -153,6 +167,12 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) { return "", nil, types.NewError(types.ErrIOFailure, fmt.Sprintf("error reading from stdin: %v", err), "") } + if cmd != "VERSION" { + if err := validateConfig(stdinData); err != nil { + return "", nil, err + } + } + cmdArgs := &CmdArgs{ ContainerID: contID, Netns: netns, @@ -219,18 +239,6 @@ func (t *dispatcher) pluginMain(funcs CNIFuncs, versionInfo version.PluginInfo, return err } - if cmd != "VERSION" { - if err = validateConfig(cmdArgs.StdinData); err != nil { - return err - } - if err = utils.ValidateContainerID(cmdArgs.ContainerID); err != nil { - return err - } - if err = utils.ValidateInterfaceName(cmdArgs.IfName); err != nil { - return err - } - } - switch cmd { case "ADD": err = t.checkVersionAndCall(cmdArgs, versionInfo, funcs.Add) @@ -301,7 +309,7 @@ func (t *dispatcher) pluginMain(funcs CNIFuncs, versionInfo version.PluginInfo, return nil } } - return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow CHECK", "") + return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow GC", "") case "VERSION": if err := versionInfo.Encode(t.Stdout); err != nil { return types.NewError(types.ErrIOFailure, err.Error(), "") @@ -325,10 +333,14 @@ func (t *dispatcher) pluginMain(funcs CNIFuncs, versionInfo version.PluginInfo, // // To let this package automatically handle errors and call os.Exit(1) for you, // use PluginMain() instead. +// +// Deprecated: Use github.com/containernetworking/cni/pkg/skel.PluginMainFuncsWithError instead. func PluginMainWithError(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, versionInfo version.PluginInfo, about string) *types.Error { - return PluginMainFuncs(CNIFuncs{Add: cmdAdd, Check: cmdCheck, Del: cmdDel}, versionInfo, about) + return PluginMainFuncsWithError(CNIFuncs{Add: cmdAdd, Check: cmdCheck, Del: cmdDel}, versionInfo, about) } +// CNIFuncs contains a group of callback command funcs to be passed in as +// parameters to the core "main" for a plugin. type CNIFuncs struct { Add func(_ *CmdArgs) error Del func(_ *CmdArgs) error @@ -336,7 +348,19 @@ type CNIFuncs struct { GC func(_ *CmdArgs) error } -func PluginMainFuncs(funcs CNIFuncs, versionInfo version.PluginInfo, about string) *types.Error { +// PluginMainFuncsWithError is the core "main" for a plugin. It accepts +// callback functions defined within CNIFuncs and returns an error. +// +// The caller must also specify what CNI spec versions the plugin supports. +// +// It is the responsibility of the caller to check for non-nil error return. +// +// For a plugin to comply with the CNI spec, it must print any error to stdout +// as JSON and then exit with nonzero status code. +// +// To let this package automatically handle errors and call os.Exit(1) for you, +// use PluginMainFuncs() instead. +func PluginMainFuncsWithError(funcs CNIFuncs, versionInfo version.PluginInfo, about string) *types.Error { return (&dispatcher{ Getenv: os.Getenv, Stdin: os.Stdin, @@ -345,6 +369,28 @@ func PluginMainFuncs(funcs CNIFuncs, versionInfo version.PluginInfo, about strin }).pluginMain(funcs, versionInfo, about) } +// PluginMainFuncs is the core "main" for a plugin which includes automatic error handling. +// This is a newer alternative func to PluginMain which abstracts CNI commands within a +// CNIFuncs interface. +// +// The caller must also specify what CNI spec versions the plugin supports. +// +// The caller can specify an "about" string, which is printed on stderr +// when no CNI_COMMAND is specified. The recommended output is "CNI plugin v" +// +// When an error occurs in any func in CNIFuncs, PluginMainFuncs will print the error +// as JSON to stdout and call os.Exit(1). +// +// To have more control over error handling, use PluginMainFuncsWithError() instead. +func PluginMainFuncs(funcs CNIFuncs, versionInfo version.PluginInfo, about string) { + if e := PluginMainFuncsWithError(funcs, versionInfo, about); e != nil { + if err := e.Print(); err != nil { + log.Print("Error writing error JSON to stdout: ", err) + } + os.Exit(1) + } +} + // PluginMain is the core "main" for a plugin which includes automatic error handling. // // The caller must also specify what CNI spec versions the plugin supports. @@ -356,6 +402,8 @@ func PluginMainFuncs(funcs CNIFuncs, versionInfo version.PluginInfo, about strin // as JSON to stdout and call os.Exit(1). // // To have more control over error handling, use PluginMainWithError() instead. +// +// Deprecated: Use github.com/containernetworking/cni/pkg/skel.PluginMainFuncs instead. func PluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, versionInfo version.PluginInfo, about string) { if e := PluginMainWithError(cmdAdd, cmdCheck, cmdDel, versionInfo, about); e != nil { if err := e.Print(); err != nil { diff --git a/pkg/skel/skel_test.go b/pkg/skel/skel_test.go index 255aad65..3c4f864f 100644 --- a/pkg/skel/skel_test.go +++ b/pkg/skel/skel_test.go @@ -408,6 +408,10 @@ var _ = Describe("dispatching to the correct callback", func() { Context("when the CNI_COMMAND is GC", func() { BeforeEach(func() { environment["CNI_COMMAND"] = "GC" + delete(environment, "CNI_NETNS") + delete(environment, "CNI_IFNAME") + delete(environment, "CNI_CONTAINERID") + delete(environment, "CNI_ARGS") expectedCmdArgs = &CmdArgs{ Path: "/some/cni/path", @@ -415,7 +419,9 @@ var _ = Describe("dispatching to the correct callback", func() { } dispatch = &dispatcher{ - Getenv: func(key string) string { return environment[key] }, + Getenv: func(key string) string { + return environment[key] + }, Stdin: strings.NewReader(stdinData), Stdout: stdout, Stderr: stderr, @@ -461,7 +467,7 @@ var _ = Describe("dispatching to the correct callback", func() { Expect(err).To(Equal(&types.Error{ Code: types.ErrInvalidEnvironmentVariables, - Msg: "required env variables [CNI_NETNS,CNI_IFNAME,CNI_PATH] missing", + Msg: "required env variables [CNI_PATH] missing", })) }) }) @@ -509,7 +515,7 @@ var _ = Describe("dispatching to the correct callback", func() { Context("when the plugin has a bad version", func() { It("immediately returns a useful error", func() { - dispatch.Stdin = strings.NewReader(`{ "cniVersion": "0.4.0", "some": "config", "name": "test" }`) + dispatch.Stdin = strings.NewReader(`{ "cniVersion": "1.1.0", "some": "config", "name": "test" }`) versionInfo = version.PluginSupports("0.1.0", "0.2.0", "adsfasdf") err := dispatch.pluginMain(funcs, versionInfo, "") Expect(err.Code).To(Equal(types.ErrDecodingFailure)) diff --git a/plugins/test/noop/debug/debug.go b/plugins/test/noop/debug/debug.go index a3750028..f9d661db 100644 --- a/plugins/test/noop/debug/debug.go +++ b/plugins/test/noop/debug/debug.go @@ -17,9 +17,7 @@ package debug import ( "encoding/json" - "fmt" "os" - "strings" "github.com/containernetworking/cni/pkg/skel" ) @@ -42,6 +40,15 @@ type Debug struct { CmdArgs skel.CmdArgs } +// CmdLogEntry records a single CNI command as well as its args +type CmdLogEntry struct { + Command string + CmdArgs skel.CmdArgs +} + +// CmdLog records a list of CmdLogEntry received by the noop plugin +type CmdLog []CmdLogEntry + // ReadDebug will return a debug file recorded by the noop plugin func ReadDebug(debugFilePath string) (*Debug, error) { debugBytes, err := os.ReadFile(debugFilePath) @@ -74,22 +81,32 @@ func (debug *Debug) WriteDebug(debugFilePath string) error { } // WriteCommandLog appends the executed cni command to the record file -func WriteCommandLog(path string, cmd string) error { - fp, err := os.OpenFile(path, os.O_RDWR|os.O_CREATE|os.O_APPEND, 0655) +func WriteCommandLog(path string, entry CmdLogEntry) error { + buf, err := os.ReadFile(path) if err != nil { return err } - defer fp.Close() - - _, err = fmt.Fprintln(fp, cmd) - return err + var cmds CmdLog + if len(buf) > 0 { + if err = json.Unmarshal(buf, &cmds); err != nil { + return err + } + } + cmds = append(cmds, entry) + if buf, err = json.Marshal(&cmds); err != nil { + return nil + } + return os.WriteFile(path, buf, 0o644) } -func ReadCommandLog(path string) (commands []string, err error) { - b, err := os.ReadFile(path) +func ReadCommandLog(path string) (CmdLog, error) { + buf, err := os.ReadFile(path) if err != nil { - return + return nil, err + } + var cmds CmdLog + if err = json.Unmarshal(buf, &cmds); err != nil { + return nil, err } - commands = strings.Split(strings.TrimSpace(string(b)), "\n") - return + return cmds, nil } diff --git a/plugins/test/noop/main.go b/plugins/test/noop/main.go index 0ed09e30..a457dc9c 100644 --- a/plugins/test/noop/main.go +++ b/plugins/test/noop/main.go @@ -119,7 +119,14 @@ func debugBehavior(args *skel.CmdArgs, command string) error { } if netConf.CommandLog != "" { - noop_debug.WriteCommandLog(netConf.CommandLog, command) + if err = noop_debug.WriteCommandLog( + netConf.CommandLog, + noop_debug.CmdLogEntry{ + Command: command, + CmdArgs: *args, + }); err != nil { + return err + } } if debug.ReportStderr != "" { @@ -201,6 +208,10 @@ func cmdDel(args *skel.CmdArgs) error { return debugBehavior(args, "DEL") } +func cmdGC(args *skel.CmdArgs) error { + return debugBehavior(args, "GC") +} + func saveStdin() ([]byte, error) { // Read original stdin stdinData, err := io.ReadAll(os.Stdin) @@ -232,5 +243,10 @@ func main() { } supportedVersions := debugGetSupportedVersions(stdinData) - skel.PluginMain(cmdAdd, cmdCheck, cmdDel, version.PluginSupports(supportedVersions...), "CNI noop plugin v0.7.0") + skel.PluginMainFuncs(skel.CNIFuncs{ + Add: cmdAdd, + Check: cmdCheck, + Del: cmdDel, + GC: cmdGC, + }, version.PluginSupports(supportedVersions...), "CNI noop plugin v0.7.0") }