Create a central image utility package

controllers/functionconfigs/reconciler/functionconfigreconciler.go

@@ -28,7 +28,7 @@ import (
 	"github.com/kptdev/krm-functions-catalog/functions/go/starlark/starlark"
 	fnsdk "github.com/kptdev/krm-functions-sdk/go/fn"
 	configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1"
-	"github.com/kptdev/porch/pkg/util"
+	imageutil "github.com/kptdev/porch/pkg/util/image"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2"
@@ -43,12 +43,12 @@ const FunctionRunnerFinalizer = BaseFinalizer + "-function-runner"
 const ControllerFinalizer = BaseFinalizer + "-controller"
 
 type BinaryCacheEntry struct {
-	PrefixRegex string
+	PrefixRegex *regexp.Regexp
 	Tags        map[string]string
 }
 
 type BuiltInCacheEntry struct {
-	PrefixRegex string
+	PrefixRegex *regexp.Regexp
 	Process     fnsdk.ResourceListProcessor
 	Tags        []string
 }
@@ -80,7 +80,7 @@ func (s *FunctionConfigStore) UpsertFunctionConfig(name string, obj *configapi.F
 	s.functionConfigurations[name] = obj
 }
 
-func (s *FunctionConfigStore) generateRegexPattern(prefixes []string, imageName string) string {
+func (s *FunctionConfigStore) generateRegexPattern(prefixes []string) *regexp.Regexp {
 	var preparedPrefixes []string
 	for _, prefix := range prefixes {
 		if prefix == "" {
@@ -90,28 +90,18 @@ func (s *FunctionConfigStore) generateRegexPattern(prefixes []string, imageName
 		}
 	}
 
-	return "^(?:" + strings.Join(preparedPrefixes, "|") + ")$"
+	return regexp.MustCompile("^(?:" + strings.Join(preparedPrefixes, "|") + ")$")
 
 }
 
-func splitImage(image string) (name string, tag string) {
-	lastSlash := strings.LastIndex(image, "/")
-	lastColon := strings.LastIndex(image, ":")
-
-	if lastColon > lastSlash {
-		return image[:lastColon], image[lastColon+1:]
-	}
-	return image, ""
-}
-
 func (s *FunctionConfigStore) UpdateBinaryCache(_ string, obj *configapi.FunctionConfig) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
 
 	var binaryCacheEntry BinaryCacheEntry
 	binaryCacheEntry.Tags = make(map[string]string)
 	// Create a prefix Regex
-	binaryCacheEntry.PrefixRegex = s.generateRegexPattern(obj.Spec.Prefixes, obj.Spec.Image)
+	binaryCacheEntry.PrefixRegex = s.generateRegexPattern(obj.Spec.Prefixes)
 
 	abs := obj.Spec.BinaryExecutor.Path
 	if abs[0] != '/' {
@@ -149,7 +139,7 @@ func (s *FunctionConfigStore) UpdateExecCache(name string, functionConfig *confi
 		s.builtInExecutorCache[id] = BuiltInCacheEntry{
 			Process:     fn,
 			Tags:        functionConfig.Spec.GoExecutor.Tags,
-			PrefixRegex: s.generateRegexPattern(functionConfig.Spec.Prefixes, functionConfig.Spec.Image),
+			PrefixRegex: s.generateRegexPattern(functionConfig.Spec.Prefixes),
 		}
 	}
 
@@ -181,13 +171,12 @@ func (s *FunctionConfigStore) GetBinaryFromCache(image string) (string, bool) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
 
-	image, tag := splitImage(image)
-	prefixToCheck := util.GetImageRepository(image)
-	binaryStore, exists := s.binaryExecutorCache[util.GetImageName(image)]
+	parsedImage := imageutil.Parse(image)
+	prefixToCheck := parsedImage.Prefix()
+	binaryStore, exists := s.binaryExecutorCache[parsedImage.BaseName]
 	if exists {
-		regex := regexp.MustCompile(binaryStore.PrefixRegex)
-		if regex.MatchString(prefixToCheck) {
-			binaryPath, tagExists := binaryStore.Tags[tag]
+		if binaryStore.PrefixRegex.MatchString(prefixToCheck) {
+			binaryPath, tagExists := binaryStore.Tags[parsedImage.Tag]
 			if tagExists {
 				return binaryPath, true
 			}
@@ -200,30 +189,25 @@ func (s *FunctionConfigStore) GetBinaryFromCacheByConstraint(image, tag string)
 	s.mu.RLock()
 	defer s.mu.RUnlock()
 
-	baseName := util.GetImageName(image)
-	cacheEntry := s.binaryExecutorCache[baseName]
+	parsedImage := imageutil.Parse(image)
+	cacheEntry, ok := s.binaryExecutorCache[parsedImage.BaseName]
+	if !ok {
+		return "", false
+	}
 
-	cacheKeys := make([]string, 0, len(s.binaryExecutorCache))
-	for k := range cacheEntry.Tags {
-		cacheKeys = append(cacheKeys, k)
+	if !cacheEntry.PrefixRegex.MatchString(parsedImage.Prefix()) {
+		return "", false
 	}
 
-	selectedKey, err := util.FindBestSemverMatch(tag, image, cacheKeys)
+	cacheKeys := slices.Collect(maps.Keys(cacheEntry.Tags))
+
+	selectedKey, err := imageutil.FindBestSemverMatch(tag, cacheKeys)
 	if err != nil {
 		return "", false
 	}
-	selectedBinary := cacheEntry.Tags[selectedKey]
-
-	prefixToCheck, tag := splitImage(image)
-	regex := regexp.MustCompile(cacheEntry.PrefixRegex)
-	if regex.MatchString(prefixToCheck) {
-		binaryPath, tagExists := cacheEntry.Tags[tag]
-		if tagExists {
-			return binaryPath, true
-		}
-	}
+	selectedBinary, ok := cacheEntry.Tags[selectedKey]
 
-	return selectedBinary, true
+	return selectedBinary, ok
 }
 
 func (s *FunctionConfigStore) GetExecCache() map[string]BuiltInCacheEntry {
@@ -236,16 +220,14 @@ func (s *FunctionConfigStore) GetExecCache() map[string]BuiltInCacheEntry {
 func (s *FunctionConfigStore) GetProcessorFromCache(image string) (fnsdk.ResourceListProcessor, bool) {
 	s.mu.RLock()
 	defer s.mu.RUnlock()
-	baseName := util.GetImageName(image)
-	tag := util.GetImageTag(image)
-	entry, found := s.builtInExecutorCache[baseName]
-	prefixToCheck := util.GetImageRepository(image)
+	parsedImage := imageutil.Parse(image)
+	entry, found := s.builtInExecutorCache[parsedImage.BaseName]
+	prefixToCheck := parsedImage.Prefix()
 	if prefixToCheck == "" {
 		prefixToCheck = s.defaultImagePrefix
 	}
-	if slices.Contains(entry.Tags, tag) {
-		regex := regexp.MustCompile(entry.PrefixRegex)
-		if regex.MatchString(prefixToCheck) {
+	if slices.Contains(entry.Tags, parsedImage.Tag) {
+		if entry.PrefixRegex.MatchString(prefixToCheck) {
 			return entry.Process, found
 		}
 	}

controllers/functionconfigs/reconciler/functionconfigreconciler_test.go

@@ -279,6 +279,77 @@ func TestFinalizersAdded(t *testing.T) {
 	}
 }
 
+func TestGetBinaryFromCacheByConstraint(t *testing.T) {
+	store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
+
+	obj := &configapi.FunctionConfig{
+		ObjectMeta: metav1.ObjectMeta{Name: "set-image", Namespace: testNamespace},
+		Spec: configapi.FunctionConfigSpec{
+			Image:    "set-image",
+			Prefixes: []string{""},
+			BinaryExecutor: &configapi.BinaryExecutorConfig{
+				Tags: []string{"v0.1.2", "v0.1.3"},
+				Path: "set-image",
+			},
+		},
+	}
+	store.UpdateBinaryCache(obj.Name, obj)
+
+	const expectedPath = "/functions/set-image"
+	const qualifiedImage = "ghcr.io/kptdev/krm-functions-catalog/set-image"
+
+	tests := map[string]struct {
+		image      string
+		constraint string
+		wantPath   string
+		wantFound  bool
+	}{
+		"selects highest matching version": {
+			image:      qualifiedImage,
+			constraint: ">= 0.1.2 < 0.2.0",
+			wantPath:   expectedPath,
+			wantFound:  true,
+		},
+		"prefix mismatch": {
+			image:      "evil.registry/set-image",
+			constraint: ">= 0.1.2 < 0.2.0",
+			wantFound:  false,
+		},
+		"unknown image basename": {
+			image:      "ghcr.io/kptdev/krm-functions-catalog/nonexistent",
+			constraint: ">= 0.1.0",
+			wantFound:  false,
+		},
+		"invalid semver constraint": {
+			image:      qualifiedImage,
+			constraint: ">> 1.0.0",
+			wantFound:  false,
+		},
+		"no matching version for valid constraint": {
+			image:      qualifiedImage,
+			constraint: "> 1.0.0",
+			wantFound:  false,
+		},
+		"short form without registry prefix": {
+			image:      "set-image",
+			constraint: ">= 0.1.2",
+			wantFound:  false,
+		},
+	}
+
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			path, found := store.GetBinaryFromCacheByConstraint(tc.image, tc.constraint)
+			assert.Equal(t, tc.wantFound, found)
+			if tc.wantFound {
+				assert.Equal(t, tc.wantPath, path)
+			} else {
+				assert.Empty(t, path)
+			}
+		})
+	}
+}
+
 func TestGetProcessorFromCache(t *testing.T) {
 	store := NewFunctionConfigStore(defaultImagePrefix, functionCacheDir)
 

func/internal/executableevaluator.go

@@ -66,7 +66,7 @@ func (e *executableEvaluator) EvaluateFunction(ctx context.Context, req *pb.Eval
 		}
 		selectedBinary = binary
 	} else {
-		klog.Infof("Image tag is empty, using the image with explicit tag: %q", req.Image)
+		klog.V(2).Infof("Image tag is empty, using the image with explicit tag: %q", req.Image)
 		binary, exists := e.FunctionConfigStore.GetBinaryFromCache(req.Image)
 		if !exists {
 			return nil, &fn.NotFoundError{

func/internal/executableevaluator_test.go

@@ -16,16 +16,18 @@ package internal
 
 import (
 	"bytes"
+	"flag"
 	"fmt"
 	"os"
+	"path/filepath"
 	"testing"
 
 	kptfilev1 "github.com/kptdev/kpt/api/kptfile/v1"
 	"github.com/kptdev/kpt/pkg/fn"
 	configapi "github.com/kptdev/porch/api/porchconfig/v1alpha1"
 	"github.com/kptdev/porch/controllers/functionconfigs/reconciler"
 	pb "github.com/kptdev/porch/func/evaluator"
-	"github.com/kptdev/porch/pkg/util"
+	imageutil "github.com/kptdev/porch/pkg/util/image"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"k8s.io/klog/v2"
@@ -87,6 +89,10 @@ func TestNewExecutableEvaluator(t *testing.T) {
 }
 
 func TestEvaluateExecutableFunction(t *testing.T) {
+	flagSet := flag.NewFlagSet("log-level", flag.ContinueOnError)
+	klog.InitFlags(flagSet)
+	_ = flagSet.Parse([]string{"--v", "3"})
+
 	const tempCacheDir = "/tmp/func_cache"
 	t.Run("invalid semver constraint will cause function not found error", func(t *testing.T) {
 		ctx := t.Context()
@@ -96,7 +102,7 @@ func TestEvaluateExecutableFunction(t *testing.T) {
 
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte("req-rl"),
-			Image:        util.ImageJoin(defaultKRMImagePrefix, testImageName),
+			Image:        imageutil.Join(defaultKRMImagePrefix, testImageName),
 			Tag:          ">> 0.1.3 < 0.2.0", // Invalid semver constraint, '>>' is not a valid operator
 		}
 
@@ -118,7 +124,7 @@ func TestEvaluateExecutableFunction(t *testing.T) {
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte("req-rl"),
 			// This image is not included in the config.yaml -> function not found
-			Image: util.ImageJoin(defaultKRMImagePrefix, testImageName),
+			Image: imageutil.Join(defaultKRMImagePrefix, testImageName),
 			Tag:   "> 0.1.3 < 0.2.0", // This is a valid semver constraint syntax
 		}
 
@@ -135,7 +141,7 @@ func TestEvaluateExecutableFunction(t *testing.T) {
 
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte("req-rl"),
-			Image:        util.ImageJoin(defaultKRMImagePrefix, setImageFunction),
+			Image:        imageutil.Join(defaultKRMImagePrefix, setImageFunction),
 			Tag:          "> 0.1.3 < 0.2.0",
 		}
 
@@ -152,7 +158,7 @@ func TestEvaluateExecutableFunction(t *testing.T) {
 
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte("req-rl"),
-			Image:        util.ImageJoin(defaultKRMImagePrefix, setImageFunction),
+			Image:        imageutil.Join(defaultKRMImagePrefix, setImageFunction),
 			Tag:          ">= 0.1.2 < 0.2.0",
 		}
 
@@ -169,7 +175,7 @@ func TestEvaluateExecutableFunction(t *testing.T) {
 		tmpDir := t.TempDir()
 
 		// Create a simple test executable that echoes input as a valid KRM function
-		testBinary := util.ImageJoin(tmpDir, setImageFunction)
+		testBinary := filepath.Join(tmpDir, setImageFunction)
 		const testScript = `#!/bin/sh
 # Emulating the KRM function execution by running this shell script
 cat
@@ -192,7 +198,7 @@ items: []
 		// We expect v0.1.3 to be selected as it's the greatest version
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte(resourceList),
-			Image:        util.ImageJoin(defaultKRMImagePrefix, setImageFunction),
+			Image:        imageutil.Join(defaultKRMImagePrefix, setImageFunction),
 			Tag:          ">= 0.1.2 < 0.2.0",
 		}
 
@@ -221,9 +227,7 @@ items: []
 		assert.NotNil(t, resp)
 
 		// Verify the klog message contains the expected version selection
-		assert.Contains(t, logOutput, `Selected image "ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.3"`)
-		assert.Contains(t, logOutput, `(version "0.1.3")`)
-		assert.Contains(t, logOutput, `for request "ghcr.io/kptdev/krm-functions-catalog/set-image"`)
+		assert.Contains(t, logOutput, `Selected tag "v0.1.3"`)
 	})
 	t.Run("successful function execution with explicit tagging", func(t *testing.T) {
 		ctx := t.Context()
@@ -232,7 +236,7 @@ items: []
 		tmpDir := t.TempDir()
 
 		// Create a simple test executable that echoes input as a valid KRM function
-		testBinary := util.ImageJoin(tmpDir, setImageFunction)
+		testBinary := filepath.Join(tmpDir, setImageFunction)
 		const testScript = `#!/bin/sh
 # Emulating the KRM function execution by running this shell script
 cat
@@ -254,7 +258,7 @@ items: []
 		// Explicit tagging
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte(resourceList),
-			Image:        util.ImageJoin(defaultKRMImagePrefix, setImageFunction) + ":v0.1.3",
+			Image:        imageutil.Join(defaultKRMImagePrefix, setImageFunction) + ":v0.1.3",
 		}
 
 		// Capture klog output by redirecting stderr
@@ -291,7 +295,7 @@ items: []
 		tmpDir := t.TempDir()
 
 		// Create a simple test executable that echoes input as a valid KRM function
-		testBinary := util.ImageJoin(tmpDir, setImageFunction)
+		testBinary := filepath.Join(tmpDir, setImageFunction)
 		const testScript = `#!/bin/sh
 # Emulating the KRM function execution by running this shell script
 cat
@@ -312,7 +316,7 @@ items: []
 
 		req := &pb.EvaluateFunctionRequest{
 			ResourceList: []byte(resourceList),
-			Image:        util.ImageJoin(defaultKRMImagePrefix, setImageFunction) + ":v0.0.1",
+			Image:        imageutil.Join(defaultKRMImagePrefix, setImageFunction) + ":v0.0.1",
 			Tag:          ">= 0.1.2 < 0.2.0",
 		}
 
@@ -341,8 +345,6 @@ items: []
 		assert.NotNil(t, resp)
 
 		// Verify the klog message contains the expected version selection
-		assert.Contains(t, logOutput, `Selected image "ghcr.io/kptdev/krm-functions-catalog/set-image:v0.1.3"`)
-		assert.Contains(t, logOutput, `(version "0.1.3")`)
-		assert.Contains(t, logOutput, `for request "ghcr.io/kptdev/krm-functions-catalog/set-image"`)
+		assert.Contains(t, logOutput, `Selected tag "v0.1.3"`)
 	})
 }