Unify process.BinPathFinder with envtest's logic

Previously, envtest had separate logic that overrode
testing/process.BinPathFinder, mainly because the latter used to be in a
separate module.

Since they're now in the same module and BinPathFinder is internal, we
can just unify the logic and remove some of our hacks.

Author: DirectXMan12

pkg/envtest/server.go

@@ -19,7 +19,6 @@ package envtest
 import (
 	"fmt"
 	"os"
-	"path/filepath"
 	"strings"
 	"time"
 
@@ -29,6 +28,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/internal/testing/controlplane"
+	"sigs.k8s.io/controller-runtime/pkg/internal/testing/process"
 
 	logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
 )
@@ -48,40 +48,17 @@ It's possible to override some defaults, by setting the following environment va
 
 */
 const (
-	envUseExistingCluster  = "USE_EXISTING_CLUSTER"
-	envKubeAPIServerBin    = "TEST_ASSET_KUBE_APISERVER"
-	envEtcdBin             = "TEST_ASSET_ETCD"
-	envKubectlBin          = "TEST_ASSET_KUBECTL"
-	envKubebuilderPath     = "KUBEBUILDER_ASSETS"
-	envStartTimeout        = "KUBEBUILDER_CONTROLPLANE_START_TIMEOUT"
-	envStopTimeout         = "KUBEBUILDER_CONTROLPLANE_STOP_TIMEOUT"
-	envAttachOutput        = "KUBEBUILDER_ATTACH_CONTROL_PLANE_OUTPUT"
-	defaultKubebuilderPath = "/usr/local/kubebuilder/bin"
-	StartTimeout           = 60
-	StopTimeout            = 60
+	envUseExistingCluster = "USE_EXISTING_CLUSTER"
+	envStartTimeout       = "KUBEBUILDER_CONTROLPLANE_START_TIMEOUT"
+	envStopTimeout        = "KUBEBUILDER_CONTROLPLANE_STOP_TIMEOUT"
+	envAttachOutput       = "KUBEBUILDER_ATTACH_CONTROL_PLANE_OUTPUT"
+	StartTimeout          = 60
+	StopTimeout           = 60
 
 	defaultKubebuilderControlPlaneStartTimeout = 20 * time.Second
 	defaultKubebuilderControlPlaneStopTimeout  = 20 * time.Second
 )
 
-// getBinAssetPath returns a path for binary from the following list of locations,
-// ordered by precedence:
-// 0. KUBEBUILDER_ASSETS
-// 1. Environment.BinaryAssetsDirectory
-// 2. The default path, "/usr/local/kubebuilder/bin"
-func (te *Environment) getBinAssetPath(binary string) string {
-	valueFromEnvVar := os.Getenv(envKubebuilderPath)
-	if valueFromEnvVar != "" {
-		return filepath.Join(valueFromEnvVar, binary)
-	}
-
-	if te.BinaryAssetsDirectory != "" {
-		return filepath.Join(te.BinaryAssetsDirectory, binary)
-	}
-
-	return filepath.Join(defaultKubebuilderPath, binary)
-}
-
 // ControlPlane is the re-exported ControlPlane type from the internal testing package
 type ControlPlane = controlplane.ControlPlane
 
@@ -237,19 +214,9 @@ func (te *Environment) Start() (*rest.Config, error) {
 			te.ControlPlane.Etcd.Err = os.Stderr
 		}
 
-		if os.Getenv(envKubeAPIServerBin) == "" {
-			apiServer.Path = te.getBinAssetPath("kube-apiserver")
-		}
-		if os.Getenv(envEtcdBin) == "" {
-			te.ControlPlane.Etcd.Path = te.getBinAssetPath("etcd")
-		}
-		if os.Getenv(envKubectlBin) == "" {
-			// we can't just set the path manually (it's behind a function), so set the environment variable instead
-			// TODO(directxman12): re-evaluate this post pkg/internal/testing refactor
-			if err := os.Setenv(envKubectlBin, te.getBinAssetPath("kubectl")); err != nil {
-				return nil, fmt.Errorf("unable to override kubectl environment path: %w", err)
-			}
-		}
+		apiServer.Path = process.BinPathFinder("kube-apiserver", te.BinaryAssetsDirectory)
+		te.ControlPlane.Etcd.Path = process.BinPathFinder("etcd", te.BinaryAssetsDirectory)
+		te.ControlPlane.KubectlPath = process.BinPathFinder("kubectl", te.BinaryAssetsDirectory)
 
 		if err := te.defaultTimeouts(); err != nil {
 			return nil, fmt.Errorf("failed to default controlplane timeouts: %w", err)

pkg/internal/testing/controlplane/kubectl.go

@@ -82,7 +82,7 @@ type KubeCtl struct {
 // stderr.
 func (k *KubeCtl) Run(args ...string) (stdout, stderr io.Reader, err error) {
 	if k.Path == "" {
-		k.Path = process.BinPathFinder("kubectl")
+		k.Path = process.BinPathFinder("kubectl", "")
 	}
 
 	stdoutBuffer := &bytes.Buffer{}

pkg/internal/testing/controlplane/plane.go

@@ -23,6 +23,9 @@ type ControlPlane struct {
 	APIServer *APIServer
 	Etcd      *Etcd
 
+	// Kubectl will override the default asset search path for kubectl
+	KubectlPath string
+
 	// for the deprecated methods (Kubectl, etc)
 	defaultUserCfg     *rest.Config
 	defaultUserKubectl *KubeCtl
@@ -121,7 +124,7 @@ type AuthenticatedUser struct {
 
 	// apiServer is a handle to the APIServer that's used when finalizing cfg
 	// and producing the kubectl instance.
-	apiServer *APIServer
+	plane *ControlPlane
 
 	// kubectl is our existing, provisioned kubectl.  We don't provision one
 	// till someone actually asks for it.
@@ -139,12 +142,12 @@ func (u *AuthenticatedUser) Config() *rest.Config {
 	if u.cfgIsComplete {
 		return u.cfg
 	}
-	if len(u.apiServer.SecureServing.CA) == 0 {
+	if len(u.plane.APIServer.SecureServing.CA) == 0 {
 		panic("the API server has not yet been started, please do that before accessing connection details")
 	}
 
-	u.cfg.CAData = u.apiServer.SecureServing.CA
-	u.cfg.Host = u.apiServer.SecureServing.URL("https", "/").String()
+	u.cfg.CAData = u.plane.APIServer.SecureServing.CA
+	u.cfg.Host = u.plane.APIServer.SecureServing.URL("https", "/").String()
 	u.cfgIsComplete = true
 	return u.cfg
 }
@@ -168,12 +171,12 @@ func (u *AuthenticatedUser) Kubectl() (*KubeCtl, error) {
 	if u.kubectl != nil {
 		return u.kubectl, nil
 	}
-	if len(u.apiServer.CertDir) == 0 {
+	if len(u.plane.APIServer.CertDir) == 0 {
 		panic("the API server has not yet been started, please do that before accessing connection details")
 	}
 
 	// cleaning this up is handled when our tmpDir is deleted
-	out, err := os.CreateTemp(u.apiServer.CertDir, "*.kubecfg")
+	out, err := os.CreateTemp(u.plane.APIServer.CertDir, "*.kubecfg")
 	if err != nil {
 		return nil, fmt.Errorf("unable to create file for kubeconfig: %w", err)
 	}
@@ -185,7 +188,9 @@ func (u *AuthenticatedUser) Kubectl() (*KubeCtl, error) {
 	if _, err := out.Write(contents); err != nil {
 		return nil, fmt.Errorf("unable to write kubeconfig to disk at %s: %w", out.Name(), err)
 	}
-	k := &KubeCtl{}
+	k := &KubeCtl{
+		Path: u.plane.KubectlPath,
+	}
 	k.Opts = append(k.Opts, fmt.Sprintf("--kubeconfig=%s", out.Name()))
 	u.kubectl = k
 	return k, nil
@@ -214,8 +219,8 @@ func (f *ControlPlane) AddUser(user User, baseConfig *rest.Config) (*Authenticat
 	}
 
 	return &AuthenticatedUser{
-		cfg:       cfg,
-		apiServer: f.APIServer,
+		cfg:   cfg,
+		plane: f,
 	}, nil
 }
 

pkg/internal/testing/process/bin_path_finder.go

@@ -4,34 +4,51 @@ import (
 	"os"
 	"path/filepath"
 	"regexp"
-	"runtime"
 	"strings"
 )
 
-var assetsPath string
-
-func init() {
-	_, thisFile, _, ok := runtime.Caller(0)
-	if !ok {
-		panic("Could not determine the path of the BinPathFinder")
-	}
-	assetsPath = filepath.Join(filepath.Dir(thisFile), "..", "assets", "bin")
-}
-
-// TODO(directxman12): unify this with the logic from envtest
+const (
+	// EnvAssetsPath is the environment variable that stores the global test
+	// binary location override.
+	EnvAssetsPath = "KUBEBUILDER_ASSETS"
+	// EnvAssetOverridePrefix is the environment variable prefix for per-binary
+	// location overrides.
+	EnvAssetOverridePrefix = "TEST_ASSET_"
+	// AssetsDefaultPath is the default location to look for test binaries in,
+	// if no override was provided.
+	AssetsDefaultPath = "/usr/local/kubebuilder/bin"
+)
 
-// BinPathFinder checks the an environment variable, derived from the symbolic name,
-// and falls back to a default assets location when this variable is not set
-func BinPathFinder(symbolicName string) (binPath string) {
+// BinPathFinder finds the path to the given named binary, using the following locations
+// in order of precedence (highest first).  Notice that the various env vars only need
+// to be set -- the asset is not checked for existence on the filesystem.
+//
+// 1. TEST_ASSET_{tr/a-z-/A-Z_/} (if set; asset overrides -- EnvAssetOverridePrefix)
+// 1. KUBEBUILDER_ASSETS (if set; global asset path -- EnvAssetsPath)
+// 3. assetDirectory (if set; per-config asset directory)
+// 4. /usr/local/kubebuilder/bin (AssetsDefaultPath)
+func BinPathFinder(symbolicName, assetDirectory string) (binPath string) {
 	punctuationPattern := regexp.MustCompile("[^A-Z0-9]+")
 	sanitizedName := punctuationPattern.ReplaceAllString(strings.ToUpper(symbolicName), "_")
 	leadingNumberPattern := regexp.MustCompile("^[0-9]+")
 	sanitizedName = leadingNumberPattern.ReplaceAllString(sanitizedName, "")
-	envVar := "TEST_ASSET_" + sanitizedName
+	envVar := EnvAssetOverridePrefix + sanitizedName
 
+	// TEST_ASSET_XYZ
 	if val, ok := os.LookupEnv(envVar); ok {
 		return val
 	}
 
-	return filepath.Join(assetsPath, symbolicName)
+	// KUBEBUILDER_ASSETS
+	if val, ok := os.LookupEnv(EnvAssetsPath); ok {
+		return filepath.Join(val, symbolicName)
+	}
+
+	// assetDirectory
+	if assetDirectory != "" {
+		return filepath.Join(assetDirectory, symbolicName)
+	}
+
+	// default path
+	return filepath.Join(AssetsDefaultPath, symbolicName)
 }

pkg/internal/testing/process/bin_path_finder_test.go

@@ -8,59 +8,53 @@ import (
 )
 
 var _ = Describe("BinPathFinder", func() {
-	Context("when relying on the default assets path", func() {
-		var (
-			previousAssetsPath string
-		)
+	var prevAssetPath string
+	BeforeEach(func() {
+		prevAssetPath = os.Getenv(EnvAssetsPath)
+		Expect(os.Unsetenv(EnvAssetsPath)).To(Succeed())
+		Expect(os.Unsetenv(EnvAssetOverridePrefix + "_SOME_FAKE"))
+		Expect(os.Unsetenv(EnvAssetOverridePrefix + "OTHERFAKE"))
+	})
+	AfterEach(func() {
+		if prevAssetPath != "" {
+			Expect(os.Setenv(EnvAssetsPath, prevAssetPath))
+		}
+	})
+	Context("when individual overrides are present", func() {
 		BeforeEach(func() {
-			previousAssetsPath = assetsPath
-			assetsPath = "/some/path/assets/bin"
+			Expect(os.Setenv(EnvAssetOverridePrefix+"OTHERFAKE", "/other/path")).To(Succeed())
+			Expect(os.Setenv(EnvAssetOverridePrefix+"_SOME_FAKE", "/some/path")).To(Succeed())
+			// set the global path to make sure we don't prefer it
+			Expect(os.Setenv(EnvAssetsPath, "/global/path")).To(Succeed())
 		})
-		AfterEach(func() {
-			assetsPath = previousAssetsPath
+
+		It("should prefer individual overrides, using them unmodified", func() {
+			Expect(BinPathFinder("otherfake", "/hardcoded/path")).To(Equal("/other/path"))
 		})
-		It("returns the default path when no env var is configured", func() {
-			binPath := BinPathFinder("some_bin")
-			Expect(binPath).To(Equal("/some/path/assets/bin/some_bin"))
+
+		It("should convert lowercase to uppercase, remove leading numbers, and replace punctuation with underscores when resolving the env var name", func() {
+			Expect(BinPathFinder("123.some-fake", "/hardcoded/path")).To(Equal("/some/path"))
 		})
 	})
 
-	Context("when environment is configured", func() {
-		var (
-			previousValue string
-			wasSet        bool
-		)
+	Context("when individual overrides are missing but the global override is present", func() {
 		BeforeEach(func() {
-			envVarName := "TEST_ASSET_ANOTHER_SYMBOLIC_NAME"
-			if val, ok := os.LookupEnv(envVarName); ok {
-				previousValue = val
-				wasSet = true
-			}
-			os.Setenv(envVarName, "/path/to/some_bin.exe")
+			Expect(os.Setenv(EnvAssetsPath, "/global/path")).To(Succeed())
 		})
-		AfterEach(func() {
-			if wasSet {
-				os.Setenv("TEST_ASSET_ANOTHER_SYMBOLIC_NAME", previousValue)
-			} else {
-				os.Unsetenv("TEST_ASSET_ANOTHER_SYMBOLIC_NAME")
-			}
+		It("should prefer the global override, appending the name to that path", func() {
+			Expect(BinPathFinder("some-fake", "/hardcoded/path")).To(Equal("/global/path/some-fake"))
 		})
-		It("returns the path from the env", func() {
-			binPath := BinPathFinder("another_symbolic_name")
-			Expect(binPath).To(Equal("/path/to/some_bin.exe"))
+	})
+
+	Context("when an asset directory is given and no overrides are present", func() {
+		It("should use the asset directory, appending the name to that path", func() {
+			Expect(BinPathFinder("some-fake", "/hardcoded/path")).To(Equal("/hardcoded/path/some-fake"))
 		})
+	})
 
-		It("sanitizes the environment variable name", func() {
-			By("cleaning all non-underscore punctuation")
-			binPath := BinPathFinder("another-symbolic name")
-			Expect(binPath).To(Equal("/path/to/some_bin.exe"))
-			binPath = BinPathFinder("another+symbolic\\name")
-			Expect(binPath).To(Equal("/path/to/some_bin.exe"))
-			binPath = BinPathFinder("another=symbolic.name")
-			Expect(binPath).To(Equal("/path/to/some_bin.exe"))
-			By("removing numbers from the beginning of the name")
-			binPath = BinPathFinder("12another_symbolic_name")
-			Expect(binPath).To(Equal("/path/to/some_bin.exe"))
+	Context("when no path configuration is given", func() {
+		It("should just use the default path", func() {
+			Expect(BinPathFinder("some-fake", "")).To(Equal("/usr/local/kubebuilder/bin/some-fake"))
 		})
 	})
 })

pkg/internal/testing/process/process.go

@@ -89,7 +89,7 @@ func (ps *State) Init(name string) error {
 		if name == "" {
 			return fmt.Errorf("must have at least one of name or path")
 		}
-		ps.Path = BinPathFinder(name)
+		ps.Path = BinPathFinder(name, "")
 	}
 
 	if ps.Dir == "" {