From fb891767e3fa03c45869617faa99cfc9372ea170 Mon Sep 17 00:00:00 2001
From: Gordon Bleux <33967640+UiP9AV6Y@users.noreply.github.com>
Date: Mon, 14 Nov 2022 12:20:23 +0100
Subject: [PATCH] cnitool: deduplicate environment variable definitions

create a reusable package for environment variable to be used
by `cnitool` and the other library code.

Signed-off-by: Gordon Bleux <33967640+UiP9AV6Y@users.noreply.github.com>
---
 cnitool/cnitool.go        |  41 ++++-----------
 pkg/env/env.go            |  95 +++++++++++++++++++++++++++++++++++
 pkg/env/env_test.go       |  77 ++++++++++++++++++++++++++++
 pkg/invoke/args.go        |  34 +++++++------
 pkg/invoke/delegate.go    |   5 +-
 pkg/skel/skel.go          | 103 ++++++++++++++++++--------------------
 plugins/test/noop/main.go |   9 ++--
 7 files changed, 255 insertions(+), 109 deletions(-)
 create mode 100644 pkg/env/env.go
 create mode 100644 pkg/env/env_test.go

diff --git a/cnitool/cnitool.go b/cnitool/cnitool.go
index 0ba918383..a5d30f6e2 100644
--- a/cnitool/cnitool.go
+++ b/cnitool/cnitool.go
@@ -24,21 +24,7 @@ import (
 	"strings"
 
 	"github.com/containernetworking/cni/libcni"
-)
-
-// Protocol parameters are passed to the plugins via OS environment variables.
-const (
-	EnvCNIPath        = "CNI_PATH"
-	EnvNetDir         = "NETCONFPATH"
-	EnvCapabilityArgs = "CAP_ARGS"
-	EnvCNIArgs        = "CNI_ARGS"
-	EnvCNIIfname      = "CNI_IFNAME"
-
-	DefaultNetDir = "/etc/cni/net.d"
-
-	CmdAdd   = "add"
-	CmdCheck = "check"
-	CmdDel   = "del"
+	"github.com/containernetworking/cni/pkg/env"
 )
 
 func parseArgs(args string) ([][2]string, error) {
@@ -62,17 +48,14 @@ func main() {
 		usage()
 	}
 
-	netdir := os.Getenv(EnvNetDir)
-	if netdir == "" {
-		netdir = DefaultNetDir
-	}
+	netdir := env.GetNetDir()
 	netconf, err := libcni.LoadConfList(netdir, os.Args[2])
 	if err != nil {
 		exit(err)
 	}
 
 	var capabilityArgs map[string]interface{}
-	capabilityArgsValue := os.Getenv(EnvCapabilityArgs)
+	capabilityArgsValue := env.GetCapabilityArgs()
 	if len(capabilityArgsValue) > 0 {
 		if err = json.Unmarshal([]byte(capabilityArgsValue), &capabilityArgs); err != nil {
 			exit(err)
@@ -80,7 +63,7 @@ func main() {
 	}
 
 	var cniArgs [][2]string
-	args := os.Getenv(EnvCNIArgs)
+	args := env.GetCNIArgs()
 	if len(args) > 0 {
 		cniArgs, err = parseArgs(args)
 		if err != nil {
@@ -88,11 +71,7 @@ func main() {
 		}
 	}
 
-	ifName, ok := os.LookupEnv(EnvCNIIfname)
-	if !ok {
-		ifName = "eth0"
-	}
-
+	ifName := env.GetCNIIfname()
 	netns := os.Args[3]
 	netns, err = filepath.Abs(netns)
 	if err != nil {
@@ -103,7 +82,7 @@ func main() {
 	s := sha512.Sum512([]byte(netns))
 	containerID := fmt.Sprintf("cnitool-%x", s[:10])
 
-	cninet := libcni.NewCNIConfig(filepath.SplitList(os.Getenv(EnvCNIPath)), nil)
+	cninet := libcni.NewCNIConfig(env.ParseCNIPath(), nil)
 
 	rt := &libcni.RuntimeConf{
 		ContainerID:    containerID,
@@ -113,17 +92,17 @@ func main() {
 		CapabilityArgs: capabilityArgs,
 	}
 
-	switch os.Args[1] {
-	case CmdAdd:
+	switch strings.ToUpper(os.Args[1]) {
+	case env.CmdAdd:
 		result, err := cninet.AddNetworkList(context.TODO(), netconf, rt)
 		if result != nil {
 			_ = result.Print()
 		}
 		exit(err)
-	case CmdCheck:
+	case env.CmdCheck:
 		err := cninet.CheckNetworkList(context.TODO(), netconf, rt)
 		exit(err)
-	case CmdDel:
+	case env.CmdDel:
 		exit(cninet.DelNetworkList(context.TODO(), netconf, rt))
 	}
 }
diff --git a/pkg/env/env.go b/pkg/env/env.go
new file mode 100644
index 000000000..9fdab2b48
--- /dev/null
+++ b/pkg/env/env.go
@@ -0,0 +1,95 @@
+// Copyright 2015 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.
+
+package env
+
+import (
+	"os"
+	"path/filepath"
+)
+
+// Protocol parameters are passed to the plugins via OS environment variables.
+const (
+	VarCapabilityArgs   = "CAP_ARGS"
+	VarCNIArgs          = "CNI_ARGS"
+	VarCNICommand       = "CNI_COMMAND"
+	VarCNIContainerId   = "CNI_CONTAINERID"
+	VarCNIIfname        = "CNI_IFNAME"
+	VarCNINetNs         = "CNI_NETNS"
+	VarCNINetNsOverride = "CNI_NETNS_OVERRIDE"
+	VarCNIPath          = "CNI_PATH"
+	VarNetDir           = "NETCONFPATH"
+
+	DefaultCapabilityArgs = ""
+	DefaultCNIArgs        = ""
+	DefaultCNIIfname      = "eth0"
+	DefaultNetDir         = "/etc/cni/net.d"
+	DefaultCNIPath        = ""
+
+	// supported CNI_COMMAND values
+
+	CmdAdd     = "ADD"
+	CmdCheck   = "CHECK"
+	CmdDel     = "DEL"
+	CmdVersion = "VERSION"
+)
+
+// GetValue is a wrapper around os.GetEnv, which
+// returns the given fallback value in case the
+// environment variable is not set or an empty
+// string.
+func GetValue(key, fallbackValue string) string {
+	val := os.Getenv(key)
+	if val == "" {
+		return fallbackValue
+	}
+
+	return val
+}
+
+// GetNetDir gets the network configuration
+// directory setting from the environment
+func GetNetDir() string {
+	return GetValue(VarNetDir, DefaultNetDir)
+}
+
+// GetCapabilityArgs gets the capability
+// settings from the environment
+func GetCapabilityArgs() string {
+	return GetValue(VarCapabilityArgs, DefaultCapabilityArgs)
+}
+
+// GetCNIArgs gets the plugin arguments
+// from the environment
+func GetCNIArgs() string {
+	return GetValue(VarCNIArgs, DefaultCNIArgs)
+}
+
+// GetCNIIfname gets the network interface
+// setting from the environment
+func GetCNIIfname() string {
+	return GetValue(VarCNIIfname, DefaultCNIIfname)
+}
+
+// GetCNIPath gets the plugin lookup path
+// from the environment
+func GetCNIPath() string {
+	return GetValue(VarCNIPath, DefaultCNIPath)
+}
+
+// ParseCNIPath returns a list of directories to
+// search for executables
+func ParseCNIPath() []string {
+	return filepath.SplitList(GetCNIPath())
+}
diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go
new file mode 100644
index 000000000..217667490
--- /dev/null
+++ b/pkg/env/env_test.go
@@ -0,0 +1,77 @@
+// Copyright 2016 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.
+
+package env_test
+
+import (
+	"os"
+	"strings"
+
+	"github.com/containernetworking/cni/pkg/env"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("GetValue", func() {
+	BeforeEach(func() {
+		os.Setenv("FOO", "bar")
+		os.Unsetenv("BAC")
+	})
+
+	Context("when environment has variable", func() {
+		It("returns the variable value", func() {
+			val := env.GetValue("FOO", "gaff")
+			Expect(val).To(Equal("bar"))
+		})
+	})
+
+	Context("when environment does not have variable", func() {
+		It("returns the fallback value", func() {
+			val := env.GetValue("BAC", "gaff")
+			Expect(val).To(Equal("gaff"))
+		})
+	})
+})
+
+var _ = Describe("ParseCNIPath", func() {
+	BeforeEach(func() {
+		os.Unsetenv("CNI_PATH")
+	})
+
+	AfterEach(func() {
+		os.Unsetenv("CNI_PATH")
+	})
+
+	Context("when no directories are specified", func() {
+		It("returns an empty list", func() {
+			val := env.ParseCNIPath()
+			Expect(val).To(BeEmpty())
+		})
+	})
+
+	Context("when multiple directories are specified", func() {
+		It("returns the directories as a list", func() {
+			mockCNIPath("/test/bin", "/test/libexec")
+
+			val := env.ParseCNIPath()
+			Expect(val).To(Equal([]string{"/test/bin", "/test/libexec"}))
+		})
+	})
+})
+
+func mockCNIPath(dir ...string) {
+	mock := strings.Join(dir, string(os.PathListSeparator))
+
+	os.Setenv(env.VarCNIPath, mock)
+}
diff --git a/pkg/invoke/args.go b/pkg/invoke/args.go
index 3cdb4bc8d..58554bc8c 100644
--- a/pkg/invoke/args.go
+++ b/pkg/invoke/args.go
@@ -18,6 +18,8 @@ import (
 	"fmt"
 	"os"
 	"strings"
+
+	"github.com/containernetworking/cni/pkg/env"
 )
 
 type CNIArgs interface {
@@ -54,7 +56,7 @@ type Args struct {
 var _ CNIArgs = &Args{}
 
 func (args *Args) AsEnv() []string {
-	env := os.Environ()
+	environ := os.Environ()
 	pluginArgsStr := args.PluginArgsStr
 	if pluginArgsStr == "" {
 		pluginArgsStr = stringify(args.PluginArgs)
@@ -62,15 +64,15 @@ func (args *Args) AsEnv() []string {
 
 	// Duplicated values which come first will be overridden, so we must put the
 	// custom values in the end to avoid being overridden by the process environments.
-	env = append(env,
-		"CNI_COMMAND="+args.Command,
-		"CNI_CONTAINERID="+args.ContainerID,
-		"CNI_NETNS="+args.NetNS,
-		"CNI_ARGS="+pluginArgsStr,
-		"CNI_IFNAME="+args.IfName,
-		"CNI_PATH="+args.Path,
+	environ = append(environ,
+		env.VarCNICommand+"="+args.Command,
+		env.VarCNIContainerId+"="+args.ContainerID,
+		env.VarCNINetNs+"="+args.NetNS,
+		env.VarCNIArgs+"="+pluginArgsStr,
+		env.VarCNIIfname+"="+args.IfName,
+		env.VarCNIPath+"="+args.Path,
 	)
-	return dedupEnv(env)
+	return dedupEnv(environ)
 }
 
 // taken from rkt/networking/net_plugin.go
@@ -94,23 +96,23 @@ type DelegateArgs struct {
 }
 
 func (d *DelegateArgs) AsEnv() []string {
-	env := os.Environ()
+	environ := os.Environ()
 
 	// The custom values should come in the end to override the existing
 	// process environment of the same key.
-	env = append(env,
-		"CNI_COMMAND="+d.Command,
+	environ = append(environ,
+		env.VarCNICommand+"="+d.Command,
 	)
-	return dedupEnv(env)
+	return dedupEnv(environ)
 }
 
 // dedupEnv returns a copy of env with any duplicates removed, in favor of later values.
 // Items not of the normal environment "key=value" form are preserved unchanged.
-func dedupEnv(env []string) []string {
-	out := make([]string, 0, len(env))
+func dedupEnv(environ []string) []string {
+	out := make([]string, 0, len(environ))
 	envMap := map[string]string{}
 
-	for _, kv := range env {
+	for _, kv := range environ {
 		// find the first "=" in environment, if not, just keep it
 		eq := strings.Index(kv, "=")
 		if eq < 0 {
diff --git a/pkg/invoke/delegate.go b/pkg/invoke/delegate.go
index 8defe4dd3..3c5faf359 100644
--- a/pkg/invoke/delegate.go
+++ b/pkg/invoke/delegate.go
@@ -16,9 +16,8 @@ package invoke
 
 import (
 	"context"
-	"os"
-	"path/filepath"
 
+	"github.com/containernetworking/cni/pkg/env"
 	"github.com/containernetworking/cni/pkg/types"
 )
 
@@ -27,7 +26,7 @@ func delegateCommon(delegatePlugin string, exec Exec) (string, Exec, error) {
 		exec = defaultExec
 	}
 
-	paths := filepath.SplitList(os.Getenv("CNI_PATH"))
+	paths := env.ParseCNIPath()
 	pluginPath, err := exec.FindInPath(delegatePlugin, paths)
 	if err != nil {
 		return "", nil, err
diff --git a/pkg/skel/skel.go b/pkg/skel/skel.go
index dfb610b0b..e9a9dc9dd 100644
--- a/pkg/skel/skel.go
+++ b/pkg/skel/skel.go
@@ -26,6 +26,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/containernetworking/cni/pkg/env"
 	"github.com/containernetworking/cni/pkg/ns"
 	"github.com/containernetworking/cni/pkg/types"
 	"github.com/containernetworking/cni/pkg/utils"
@@ -65,77 +66,54 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) {
 		reqForCmd reqForCmdEntry
 	}{
 		{
-			"CNI_COMMAND",
+			env.VarCNICommand,
 			&cmd,
-			reqForCmdEntry{
-				"ADD":   true,
-				"CHECK": true,
-				"DEL":   true,
-			},
+			newReqForCmdEntry(true, true, true),
 		},
 		{
-			"CNI_CONTAINERID",
+			env.VarCNIContainerId,
 			&contID,
-			reqForCmdEntry{
-				"ADD":   true,
-				"CHECK": true,
-				"DEL":   true,
-			},
+			newReqForCmdEntry(true, true, true),
 		},
 		{
-			"CNI_NETNS",
+			env.VarCNINetNs,
 			&netns,
-			reqForCmdEntry{
-				"ADD":   true,
-				"CHECK": true,
-				"DEL":   false,
-			},
+			newReqForCmdEntry(true, true, false),
 		},
 		{
-			"CNI_IFNAME",
+			env.VarCNIIfname,
 			&ifName,
-			reqForCmdEntry{
-				"ADD":   true,
-				"CHECK": true,
-				"DEL":   true,
-			},
+			newReqForCmdEntry(true, true, true),
 		},
 		{
-			"CNI_ARGS",
+			env.VarCNIArgs,
 			&args,
-			reqForCmdEntry{
-				"ADD":   false,
-				"CHECK": false,
-				"DEL":   false,
-			},
+			newReqForCmdEntry(false, false, false),
 		},
 		{
-			"CNI_PATH",
+			env.VarCNIPath,
 			&path,
-			reqForCmdEntry{
-				"ADD":   true,
-				"CHECK": true,
-				"DEL":   true,
-			},
+			newReqForCmdEntry(true, true, true),
 		},
 		{
-			"CNI_NETNS_OVERRIDE",
+			env.VarCNINetNsOverride,
 			&netnsOverride,
-			reqForCmdEntry{
-				"ADD":   false,
-				"CHECK": false,
-				"DEL":   false,
-			},
+			newReqForCmdEntry(false, false, false),
 		},
 	}
 
 	argsMissing := make([]string, 0)
 	for _, v := range vars {
-		*v.val = t.Getenv(v.name)
-		if *v.val == "" {
-			if v.reqForCmd[cmd] || v.name == "CNI_COMMAND" {
+		val := t.Getenv(v.name)
+		if v.name == env.VarCNICommand {
+			val = strings.ToUpper(val)
+		}
+		if val == "" {
+			if v.reqForCmd[cmd] || v.name == env.VarCNICommand {
 				argsMissing = append(argsMissing, v.name)
 			}
+		} else {
+			*v.val = val
 		}
 	}
 
@@ -144,7 +122,7 @@ func (t *dispatcher) getCmdArgsFromEnv() (string, *CmdArgs, *types.Error) {
 		return "", nil, types.NewError(types.ErrInvalidEnvironmentVariables, fmt.Sprintf("required env variables [%s] missing", joined), "")
 	}
 
-	if cmd == "VERSION" {
+	if cmd == env.CmdVersion {
 		t.Stdin = bytes.NewReader(nil)
 	}
 
@@ -206,7 +184,7 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 	cmd, cmdArgs, err := t.getCmdArgsFromEnv()
 	if err != nil {
 		// Print the about string to stderr when no command is set
-		if err.Code == types.ErrInvalidEnvironmentVariables && t.Getenv("CNI_COMMAND") == "" && about != "" {
+		if err.Code == types.ErrInvalidEnvironmentVariables && t.Getenv(env.VarCNICommand) == "" && about != "" {
 			_, _ = fmt.Fprintln(t.Stderr, about)
 			_, _ = fmt.Fprintf(t.Stderr, "CNI protocol versions supported: %s\n", strings.Join(versionInfo.SupportedVersions(), ", "))
 			return nil
@@ -214,7 +192,7 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 		return err
 	}
 
-	if cmd != "VERSION" {
+	if cmd != env.CmdVersion {
 		if err = validateConfig(cmdArgs.StdinData); err != nil {
 			return err
 		}
@@ -227,7 +205,7 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 	}
 
 	switch cmd {
-	case "ADD":
+	case env.CmdAdd:
 		err = t.checkVersionAndCall(cmdArgs, versionInfo, cmdAdd)
 		if err != nil {
 			return err
@@ -237,10 +215,11 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 			if checkErr != nil {
 				return checkErr
 			} else if isPluginNetNS {
-				return types.NewError(types.ErrInvalidNetNS, "plugin's netns and netns from CNI_NETNS should not be the same", "")
+				errMsg := fmt.Sprintf("plugin's netns and netns from %s should not be the same", env.VarCNINetNs)
+				return types.NewError(types.ErrInvalidNetNS, errMsg, "")
 			}
 		}
-	case "CHECK":
+	case env.CmdCheck:
 		configVersion, err := t.ConfVersionDecoder.Decode(cmdArgs.StdinData)
 		if err != nil {
 			return types.NewError(types.ErrDecodingFailure, err.Error(), "")
@@ -262,7 +241,7 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 			}
 		}
 		return types.NewError(types.ErrIncompatibleCNIVersion, "plugin version does not allow CHECK", "")
-	case "DEL":
+	case env.CmdDel:
 		err = t.checkVersionAndCall(cmdArgs, versionInfo, cmdDel)
 		if err != nil {
 			return err
@@ -272,15 +251,17 @@ func (t *dispatcher) pluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error,
 			if checkErr != nil {
 				return checkErr
 			} else if isPluginNetNS {
-				return types.NewError(types.ErrInvalidNetNS, "plugin's netns and netns from CNI_NETNS should not be the same", "")
+				errMsg := fmt.Sprintf("plugin's netns and netns from %s should not be the same", env.VarCNINetNs)
+				return types.NewError(types.ErrInvalidNetNS, errMsg, "")
 			}
 		}
-	case "VERSION":
+	case env.CmdVersion:
 		if err := versionInfo.Encode(t.Stdout); err != nil {
 			return types.NewError(types.ErrIOFailure, err.Error(), "")
 		}
 	default:
-		return types.NewError(types.ErrInvalidEnvironmentVariables, fmt.Sprintf("unknown CNI_COMMAND: %v", cmd), "")
+		errMsg := fmt.Sprintf("unknown %s: %v", env.VarCNICommand, cmd)
+		return types.NewError(types.ErrInvalidEnvironmentVariables, errMsg, "")
 	}
 
 	return err
@@ -326,3 +307,15 @@ func PluginMain(cmdAdd, cmdCheck, cmdDel func(_ *CmdArgs) error, versionInfo ver
 		os.Exit(1)
 	}
 }
+
+// newReqForCmdEntry is a helper method to create a command
+// requirements map with constants for keys
+func newReqForCmdEntry(add, check, del bool) reqForCmdEntry {
+	result := reqForCmdEntry{}
+
+	result[env.CmdAdd] = add
+	result[env.CmdCheck] = check
+	result[env.CmdDel] = del
+
+	return result
+}
diff --git a/plugins/test/noop/main.go b/plugins/test/noop/main.go
index c57249134..1642e18e4 100644
--- a/plugins/test/noop/main.go
+++ b/plugins/test/noop/main.go
@@ -29,6 +29,7 @@ import (
 	"os"
 	"strings"
 
+	"github.com/containernetworking/cni/pkg/env"
 	"github.com/containernetworking/cni/pkg/skel"
 	"github.com/containernetworking/cni/pkg/types"
 	current "github.com/containernetworking/cni/pkg/types/100"
@@ -164,7 +165,7 @@ func debugBehavior(args *skel.CmdArgs, command string) error {
 
 func debugGetSupportedVersions(stdinData []byte) []string {
 	vers := []string{"0.-42.0", "0.1.0", "0.2.0", "0.3.0", "0.3.1", "0.4.0", "1.0.0"}
-	cniArgs := os.Getenv("CNI_ARGS")
+	cniArgs := env.GetCNIArgs()
 	if cniArgs == "" {
 		return vers
 	}
@@ -185,15 +186,15 @@ func debugGetSupportedVersions(stdinData []byte) []string {
 }
 
 func cmdAdd(args *skel.CmdArgs) error {
-	return debugBehavior(args, "ADD")
+	return debugBehavior(args, env.CmdAdd)
 }
 
 func cmdCheck(args *skel.CmdArgs) error {
-	return debugBehavior(args, "CHECK")
+	return debugBehavior(args, env.CmdCheck)
 }
 
 func cmdDel(args *skel.CmdArgs) error {
-	return debugBehavior(args, "DEL")
+	return debugBehavior(args, env.CmdDel)
 }
 
 func saveStdin() ([]byte, error) {