Repository: containernetworking/cni
Pull request #802 (2020-12-21 11:00:21)

Log out the plugin name on ADD/DEL failure.
Makes it easier to diagnose conf lists when one plugin fails.


Patch:
diff --git a/libcni/api.go b/libcni/api.go
index 7e52bd8..12061c1 100644
--- a/libcni/api.go
+++ b/libcni/api.go
@@ -428,7 +428,7 @@ func (c *CNIConfig) AddNetworkList(ctx context.Context, list *NetworkConfigList,
 	for _, net := range list.Plugins {
 		result, err = c.addNetwork(ctx, list.Name, list.CNIVersion, net, result, rt)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("plugin %s failed (add): %w", pluginDescription(net.Network), err)
 		}
 	}
 
@@ -513,7 +513,7 @@ func (c *CNIConfig) DelNetworkList(ctx context.Context, list *NetworkConfigList,
 	for i := len(list.Plugins) - 1; i >= 0; i-- {
 		net := list.Plugins[i]
 		if err := c.delNetwork(ctx, list.Name, list.CNIVersion, net, cachedResult, rt); err != nil {
-			return err
+			return fmt.Errorf("plugin %s failed (delete): %w", pluginDescription(net.Network), err)
 		}
 	}
 	_ = c.cacheDel(list.Name, rt)
@@ -521,6 +521,19 @@ func (c *CNIConfig) DelNetworkList(ctx context.Context, list *NetworkConfigList,
 	return nil
 }
 
+func pluginDescription(net *types.NetConf) string {
+	if net == nil {
+		return "<missing>"
+	}
+	pluginType := net.Type
+	out := fmt.Sprintf("type=%q", pluginType)
+	name := net.Name
+	if name != "" {
+		out += fmt.Sprintf(" name=%q", name)
+	}
+	return out
+}
+
 // AddNetwork executes the plugin with the ADD command
 func (c *CNIConfig) AddNetwork(ctx context.Context, net *NetworkConfig, rt *RuntimeConf) (types.Result, error) {
 	result, err := c.addNetwork(ctx, net.Network.Name, net.Network.CNIVersion, net, nil, rt)


Test patch:
diff --git a/libcni/api_test.go b/libcni/api_test.go
index 7fbaa10..430ed2b 100644
--- a/libcni/api_test.go
+++ b/libcni/api_test.go
@@ -18,6 +18,7 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -1040,11 +1041,12 @@ var _ = Describe("Invoking plugins", func() {
 
 				It("returns the error", func() {
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "invalid characters in containerID",
 						Details: "some-%%container-id",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 			})
 
@@ -1055,11 +1057,12 @@ var _ = Describe("Invoking plugins", func() {
 
 				It("returns the error", func() {
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidNetworkConfig,
 						Msg:     "invalid characters found in network name",
 						Details: "invalid-%%-name",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 			})
 
@@ -1068,7 +1071,7 @@ var _ = Describe("Invoking plugins", func() {
 					runtimeConfig.IfName = ""
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name is empty",
 						Details: "",
@@ -1079,7 +1082,7 @@ var _ = Describe("Invoking plugins", func() {
 					runtimeConfig.IfName = "1234567890123456"
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name is too long",
 						Details: "interface name should be less than 16 characters",
@@ -1090,55 +1093,60 @@ var _ = Describe("Invoking plugins", func() {
 					runtimeConfig.IfName = "."
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name is . or ..",
 						Details: "",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 
 				It("interface name is ..", func() {
 					runtimeConfig.IfName = ".."
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name is . or ..",
 						Details: "",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 
 				It("interface name contains invalid characters /", func() {
 					runtimeConfig.IfName = "test/test"
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name contains / or : or whitespace characters",
 						Details: "",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 
 				It("interface name contains invalid characters :", func() {
 					runtimeConfig.IfName = "test:test"
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name contains / or : or whitespace characters",
 						Details: "",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 
 				It("interface name contains invalid characters whitespace", func() {
 					runtimeConfig.IfName = "test test"
 
 					_, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(Equal(&types.Error{
+					Expect(errors.Unwrap(err)).To(Equal(&types.Error{
 						Code:    types.ErrInvalidEnvironmentVariables,
 						Msg:     "interface name contains / or : or whitespace characters",
 						Details: "",
 					}))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 			})
 
@@ -1150,7 +1158,8 @@ var _ = Describe("Invoking plugins", func() {
 				It("unmarshals and returns the error", func() {
 					result, err := cniConfig.AddNetworkList(ctx, netConfigList, runtimeConfig)
 					Expect(result).To(BeNil())
-					Expect(err).To(MatchError("plugin error: banana"))
+					Expect(errors.Unwrap(err)).To(MatchError("plugin error: banana"))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (add):"))
 				})
 				It("should not have written cache files", func() {
 					resultCacheFile := resultCacheFilePath(cacheDirPath, netConfigList.Name, runtimeConfig)
@@ -1425,7 +1434,8 @@ var _ = Describe("Invoking plugins", func() {
 				})
 				It("unmarshals and returns the error", func() {
 					err := cniConfig.DelNetworkList(ctx, netConfigList, runtimeConfig)
-					Expect(err).To(MatchError("plugin error: banana"))
+					Expect(errors.Unwrap(err)).To(MatchError("plugin error: banana"))
+					Expect(err.Error()).To(HavePrefix("plugin type=\"noop\" failed (delete):"))
 				})
 			})