fix: docker registry/repository parsing (knative#1929)

* fix: docker registry/repository parsing

Use go-containerregistry to do parsing.

Signed-off-by: Matej Vasek <mvasek@redhat.com>

* fix: use kebab-case instead of camelCase

Signed-off-by: Matej Vasek <mvasek@redhat.com>

* fix: use kebab-case instead of camelCase

Signed-off-by: Matej Vasek <mvasek@redhat.com>

---------

Signed-off-by: Matej Vasek <mvasek@redhat.com>

cmd/deploy_test.go

@@ -1198,7 +1198,7 @@ func testRegistryLoads(cmdFn commandConstructor, t *testing.T) {
 
 	f := fn.Function{
 		Root:     root,
-		Name:     "myFunc",
+		Name:     "my-func",
 		Runtime:  "go",
 		Registry: "example.com/alice",
 	}
@@ -1219,7 +1219,7 @@ func testRegistryLoads(cmdFn commandConstructor, t *testing.T) {
 		t.Fatal(err)
 	}
 
-	expected := "example.com/alice/myFunc:latest"
+	expected := "example.com/alice/my-func:latest"
 	if f.Image != expected {
 		t.Fatalf("expected image name '%v'. got %v", expected, f.Image)
 	}

pkg/functions/client.go

@@ -24,7 +24,7 @@ import (
 
 const (
 	// DefaultRegistry through which containers of functions will be shuttled.
-	DefaultRegistry = "docker.io"
+	DefaultRegistry = "index.docker.io"
 
 	// DefaultTemplate is the default function signature / environmental context
 	// of the resultant function.  All runtimes are expected to have at least

pkg/functions/client_test.go

@@ -50,7 +50,7 @@ var (
 // thus implicitly tests Create, Build and Deploy, which are exposed
 // by the client API for those who prefer manual transmissions.
 func TestClient_New(t *testing.T) {
-	root := "testdata/example.com/testNew"
+	root := "testdata/example.com/test-new"
 	defer Using(t, root)()
 
 	client := fn.New(fn.WithRegistry(TestRegistry), fn.WithVerbose(true))
@@ -210,7 +210,7 @@ func TestClient_New_RuntimeRequired(t *testing.T) {
 // TestClient_New_NameDefaults ensures that a newly created function has its name defaulted
 // to a name which can be derived from the last part of the given root path.
 func TestClient_New_NameDefaults(t *testing.T) {
-	root := "testdata/example.com/testNameDefaults"
+	root := "testdata/example.com/test-name-defaults"
 	defer Using(t, root)()
 
 	client := fn.New(fn.WithRegistry(TestRegistry))
@@ -230,7 +230,7 @@ func TestClient_New_NameDefaults(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	expected := "testNameDefaults"
+	expected := "test-name-defaults"
 	if f.Name != expected {
 		t.Fatalf("name was not defaulted. expected '%v' got '%v'", expected, f.Name)
 	}
@@ -239,7 +239,7 @@ func TestClient_New_NameDefaults(t *testing.T) {
 // TestClient_New_WritesTemplate ensures the config file and files from the template
 // are written on new.
 func TestClient_New_WritesTemplate(t *testing.T) {
-	root := "testdata/example.com/testWritesTemplate"
+	root := "testdata/example.com/test-writes-template"
 	defer Using(t, root)()
 
 	client := fn.New(fn.WithRegistry(TestRegistry))
@@ -262,7 +262,7 @@ func TestClient_New_WritesTemplate(t *testing.T) {
 // TestClient_New_ExtantAborts ensures that a directory which contains an extant
 // function does not reinitialize.
 func TestClient_New_ExtantAborts(t *testing.T) {
-	root := "testdata/example.com/testExtantAborts"
+	root := "testdata/example.com/test-extant-aborts"
 	defer Using(t, root)()
 
 	client := fn.New(fn.WithRegistry(TestRegistry))
@@ -305,7 +305,7 @@ func TestClient_New_NonemptyAborts(t *testing.T) {
 // conjunction with other tools (.envrc, etc)
 func TestClient_New_HiddenFilesIgnored(t *testing.T) {
 	// Create a directory for the function
-	root := "testdata/example.com/testHiddenFilesIgnored"
+	root := "testdata/example.com/test-hidden-files-ignored"
 	defer Using(t, root)()
 
 	client := fn.New(fn.WithRegistry(TestRegistry))
@@ -332,7 +332,7 @@ func TestClient_New_HiddenFilesIgnored(t *testing.T) {
 // See the CLI for full details, but a standard default location is
 // $HOME/.config/func/repositories/boson/go/json
 func TestClient_New_RepositoriesExtensible(t *testing.T) {
-	root := "testdata/example.com/testRepositoriesExtensible"
+	root := "testdata/example.com/test-repositories-extensible"
 	defer Using(t, root)()
 
 	client := fn.New(
@@ -479,7 +479,7 @@ func TestClient_New_RegistryRequired(t *testing.T) {
 // resultant OCI container is populated.
 func TestClient_New_ImageNamePopulated(t *testing.T) {
 	// Create the root function directory
-	root := "testdata/example.com/testDeriveImage"
+	root := "testdata/example.com/test-derive-image"
 	defer Using(t, root)()
 
 	// Create the function which calculates fields such as name and image.
@@ -508,7 +508,7 @@ func TestClient_New_ImageNamePopulated(t *testing.T) {
 // For example "alice" becomes "docker.io/alice"
 func TestClient_New_ImageRegistryDefaults(t *testing.T) {
 	// Create the root function directory
-	root := "testdata/example.com/testDeriveImageDefaultRegistry"
+	root := "testdata/example.com/test-derive-image-default-registry"
 	defer Using(t, root)()
 
 	// Create the function which calculates fields such as name and image.
@@ -533,9 +533,9 @@ func TestClient_New_ImageRegistryDefaults(t *testing.T) {
 // Deploy (and confirms expected fields calculated).
 func TestClient_New_Delegation(t *testing.T) {
 	var (
-		root          = "testdata/example.com/testNewDelegates" // .. in which to initialize
-		expectedName  = "testNewDelegates"                      // expected to be derived
-		expectedImage = "example.com/alice/testNewDelegates:latest"
+		root          = "testdata/example.com/test-new-delegates" // .. in which to initialize
+		expectedName  = "test-new-delegates"                      // expected to be derived
+		expectedImage = "example.com/alice/test-new-delegates:latest"
 		builder       = mock.NewBuilder()
 		pusher        = mock.NewPusher()
 		deployer      = mock.NewDeployer()
@@ -607,7 +607,7 @@ func TestClient_New_Delegation(t *testing.T) {
 // See TestClient_Runner for the test of the default runner implementation.
 func TestClient_Run(t *testing.T) {
 	// Create the root function directory
-	root := "testdata/example.com/testRun"
+	root := "testdata/example.com/test-run"
 	defer Using(t, root)()
 
 	// client with the mock runner and the new test function
@@ -693,7 +693,7 @@ func TestClient_Runner(t *testing.T) {
 // .func to .gitignore is an important externally visible "feature", an explicit
 // test is warranted.
 func TestClient_Run_DataDir(t *testing.T) {
-	root := "testdata/example.com/testRunDataDir"
+	root := "testdata/example.com/test-run-data-dir"
 	defer Using(t, root)()
 
 	// Create a function at root
@@ -795,9 +795,9 @@ func TestClient_RunTimeout(t *testing.T) {
 // process, erroring if run on a directory uncreated.
 func TestClient_Update(t *testing.T) {
 	var (
-		root          = "testdata/example.com/testUpdate"
-		expectedName  = "testUpdate"
-		expectedImage = "example.com/alice/testUpdate:latest"
+		root          = "testdata/example.com/test-update"
+		expectedName  = "test-update"
+		expectedImage = "example.com/alice/test-update:latest"
 		builder       = mock.NewBuilder()
 		pusher        = mock.NewPusher()
 		deployer      = mock.NewDeployerWithResult(fn.DeploymentResult{
@@ -954,8 +954,8 @@ func TestClient_Deploy_RegistryUpdate(t *testing.T) {
 // the function with the name of the function at the provided root.
 func TestClient_Remove_ByPath(t *testing.T) {
 	var (
-		root         = "testdata/example.com/testRemoveByPath"
-		expectedName = "testRemoveByPath"
+		root         = "testdata/example.com/test-remove-by-path"
+		expectedName = "test-remove-by-path"
 		remover      = mock.NewRemover()
 	)
 
@@ -993,8 +993,8 @@ func TestClient_Remove_ByPath(t *testing.T) {
 // the function with the name of the function at the provided root.
 func TestClient_Remove_DeleteAll(t *testing.T) {
 	var (
-		root              = "testdata/example.com/testRemoveDeleteAll"
-		expectedName      = "testRemoveDeleteAll"
+		root              = "testdata/example.com/test-remove-delete-all"
+		expectedName      = "test-remove-delete-all"
 		remover           = mock.NewRemover()
 		pipelinesProvider = mock.NewPipelinesProvider()
 		deleteAll         = true
@@ -1039,8 +1039,8 @@ func TestClient_Remove_DeleteAll(t *testing.T) {
 // the function with the name of the function at the provided root.
 func TestClient_Remove_Dont_DeleteAll(t *testing.T) {
 	var (
-		root              = "testdata/example.com/testRemoveDontDeleteAll"
-		expectedName      = "testRemoveDontDeleteAll"
+		root              = "testdata/example.com/test-remove-dont-delete-all"
+		expectedName      = "test-remove-dont-delete-all"
 		remover           = mock.NewRemover()
 		pipelinesProvider = mock.NewPipelinesProvider()
 		deleteAll         = false
@@ -1351,7 +1351,7 @@ func TestClient_Deploy_UnbuiltErrors(t *testing.T) {
 // TestClient_New_BuilderImagesPersisted Asserts that the client preserves user-
 // provided Builder Images
 func TestClient_New_BuildersPersisted(t *testing.T) {
-	root := "testdata/example.com/testConfiguredBuilders" // Root from which to run the test
+	root := "testdata/example.com/test-configured-builders" // Root from which to run the test
 	defer Using(t, root)()
 	client := fn.New(fn.WithRegistry(TestRegistry))
 
@@ -1388,7 +1388,7 @@ func TestClient_New_BuildersPersisted(t *testing.T) {
 // TestClient_New_BuildpacksPersisted ensures that provided buildpacks are
 // persisted on new functions.
 func TestClient_New_BuildpacksPersisted(t *testing.T) {
-	root := "testdata/example.com/testConfiguredBuildpacks" // Root from which to run the test
+	root := "testdata/example.com/test-configured-buildpacks" // Root from which to run the test
 	defer Using(t, root)()
 
 	buildpacks := []string{
@@ -1504,7 +1504,7 @@ func TestClient_Runtimes(t *testing.T) {
 // TestClient_New_Timestamp ensures that the creation timestamp is set on functions
 // which are successfully initialized using the client library.
 func TestClient_New_Timestamp(t *testing.T) {
-	root := "testdata/example.com/testCreateStamp"
+	root := "testdata/example.com/test-create-stamp"
 	defer Using(t, root)()
 
 	start := time.Now()
@@ -1526,7 +1526,7 @@ func TestClient_New_Timestamp(t *testing.T) {
 // function using a simple HTTP POST method with the invoke message as form
 // field values (as though a simple form were posted).
 func TestClient_Invoke_HTTP(t *testing.T) {
-	root := "testdata/example.com/testInvokeHTTP"
+	root := "testdata/example.com/test-invoke-http"
 	defer Using(t, root)()
 
 	// Flag indicating the function was invoked
@@ -1639,7 +1639,7 @@ func TestClient_Invoke_HTTP(t *testing.T) {
 // the invoker is sending the invocation message as a CloudEvent rather than
 // a standard HTTP form POST.
 func TestClient_Invoke_CloudEvent(t *testing.T) {
-	root := "testdata/example.com/testInvokeCloudEvent"
+	root := "testdata/example.com/test-invoke-cloud-event"
 	defer Using(t, root)()
 
 	var (
@@ -1730,7 +1730,7 @@ func TestClient_Invoke_CloudEvent(t *testing.T) {
 // TestClient_Instances ensures that when a function is run (locally) its metadata
 // is available to other clients inspecting the same function using .Instances
 func TestClient_Instances(t *testing.T) {
-	root := "testdata/example.com/testInstances"
+	root := "testdata/example.com/test-instances"
 	defer Using(t, root)()
 
 	// A mock runner

pkg/functions/function.go

@@ -10,6 +10,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/google/go-containerregistry/pkg/name"
 	"gopkg.in/yaml.v2"
 	fnlabels "knative.dev/func/pkg/k8s/labels"
 	"knative.dev/pkg/ptr"
@@ -567,24 +568,19 @@ func (f Function) ImageName() (image string, err error) {
 
 	f.Registry = strings.Trim(f.Registry, "/") // too defensive?
 
-	registryTokens := strings.Split(f.Registry, "/")
-	if len(registryTokens) == 1 { // only namespace provided: ex. 'alice'
-		image = DefaultRegistry + "/" + f.Registry + "/" + f.Name
-	} else if len(registryTokens) == 2 || len(registryTokens) == 3 {
-		// registry/namespace ('quay.io/alice') or
-		// registry/parent-namespace/namespace ('quay.io/project/alice') provided
-		image = f.Registry + "/" + f.Name
-	} else if len(registryTokens) > 3 { // the name of the image is also provided `quay.io/alice/my.function.name`
-		return "", fmt.Errorf("registry should be either 'namespace', 'registry/namespace' or 'registry/parent/namespace', the name of the image will be derived from the function name")
-	}
-
 	// Explicitly append :latest tag.  We expect source control to drive
 	// versioning, rather than rely on image tags with explicitly pinned version
 	// numbers, as is seen in many serverless solutions.  This will be updated
 	// to branch name when we add source-driven canary/ bluegreen deployments.
-
 	// For pinning to an exact container image, see ImageWithDigest
-	return image + ":latest", nil
+	refStr := f.Registry + "/" + f.Name + ":latest"
+
+	ref, err := name.ParseReference(refStr)
+	if err != nil {
+		return "", fmt.Errorf("cannot determine function image: %w", err)
+	}
+
+	return ref.Name(), nil
 }
 
 // Format yaml unmarshall error to be more human friendly.

pkg/functions/function_unit_test.go

@@ -140,29 +140,38 @@ func TestFunction_ImageName(t *testing.T) {
 		err error
 	)
 	tests := []struct {
-		registry      string
 		name          string
+		registry      string
+		funcName      string
 		expectedImage string
 		expectError   bool
 	}{
-		{"alice", "myfunc", DefaultRegistry + "/alice/myfunc:latest", false},
-		{"quay.io/alice", "myfunc", "quay.io/alice/myfunc:latest", false},
-		{"docker.io/alice", "myfunc", "docker.io/alice/myfunc:latest", false},
-		{"docker.io/alice/sub", "myfunc", "docker.io/alice/sub/myfunc:latest", false},
-		{"alice", "", "", true},
-		{"", "myfunc", "", true},
+		{"short-name", "alice", "myfunc", DefaultRegistry + "/alice/myfunc:latest", false},
+		{"short-name-trailing-slash", "alice/", "myfunc", DefaultRegistry + "/alice/myfunc:latest", false},
+		{"full-name-quay-io", "quay.io/alice", "myfunc", "quay.io/alice/myfunc:latest", false},
+		{"full-name-docker-io", "docker.io/alice", "myfunc", DefaultRegistry + "/alice/myfunc:latest", false},
+		{"full-name-with-sub-path", "docker.io/alice/sub", "myfunc", DefaultRegistry + "/alice/sub/myfunc:latest", false},
+		{"localhost-direct", "localhost:5000", "myfunc", "localhost:5000/myfunc:latest", false},
+		{"full-name-with-sub-sub-path", "us-central1-docker.pkg.dev/my-gcpproject/team/user", "myfunc", "us-central1-docker.pkg.dev/my-gcpproject/team/user/myfunc:latest", false},
+		{"missing-func-name", "alice", "", "", true},
+		{"missing-registry", "", "myfunc", "", true},
 	}
 	for _, test := range tests {
-		f = Function{Registry: test.registry, Name: test.name}
-		got, err = f.ImageName()
-		if test.expectError && err == nil {
-			t.Errorf("registry '%v' and name '%v' did not yield the expected error",
-				test.registry, test.name)
-		}
-		if got != test.expectedImage {
-			t.Errorf("expected registry '%v' name '%v' to yield image '%v', got '%v'",
-				test.registry, test.name, test.expectedImage, got)
-		}
+		t.Run(test.name, func(t *testing.T) {
+			f = Function{Registry: test.registry, Name: test.funcName}
+			got, err = f.ImageName()
+			if test.expectError && err == nil {
+				t.Errorf("registry '%v' and name '%v' did not yield the expected error",
+					test.registry, test.funcName)
+			}
+			if !test.expectError && err != nil {
+				t.Errorf("unexpected error: %v", err)
+			}
+			if got != test.expectedImage {
+				t.Errorf("expected registry '%v' name '%v' to yield image '%v', got '%v'",
+					test.registry, test.funcName, test.expectedImage, got)
+			}
+		})
 	}
 }