Fix: farm build can't handle registry/path:tag

Fixes: #25039

Contrary to its specified behaviour (https://docs.podman.io/en/stable/markdown/podman-farm-build.1.html),
`podman farm build` *cannot* handle the more extended form of `--tag` that might be passed to it. Whilst
`--tag`s of the form `registry/repository` are handled ok, instances of the form `registry/repository:tag`
cause the farm to fail when attempting to push the images it has built to the registry.

This commit addresses that issue.

The primary cause (in list_builder.go) is the indiscriminate appending of the `UnknownDigestSuffix`
to ALL forms of the supplied `--tag` prior to pushing its associated images to the registry. This suffix
exists specifically to allow images to be pushed without either a digest or :tag, so attaching it
to an image actually bearing a :tag is a logical contradiction, and the construct subsequently gets
caught and kicked out by `docker_transport.go`.

This  solution:

* amends list_builder.go as follows:

  - the listLocal struct is refactored so that the provided `--tag` is held as a valid
    `v5/docker/reference.Named` rather than as a plain string. The  `--tag` string is then
    made available to the program via two new member functions. It is made available in two
    forms: a longer reference format (`registry/repository[:tag]`) and a shorter name format
    (`registry/repository`). Some parts of the existing processing require the long `reference` form,
    and others the short `name` form. The remainder of code has thus been amended to use the
    contextually appropriate form;

  - newManifestListBuilder() is amended so that it throws an errors if the `--tag` it is
    to be build upon can't be transformed to a valid Reference.Named.

* amends farm.go to handle any new error thrown by the above;

* amends farm_build_test.go to provide additional test cases to exercise the above.

Signed-off-by: Kevin Crocombe <kevin.crocombe@pegortech.co.uk>

Author: kcrocombe

pkg/farm/farm.go

@@ -187,6 +187,7 @@ func (f *Farm) Schedule(ctx context.Context, platforms []string) (Schedule, erro
 					emulated[e] = name
 				}
 			}
+
 			return nil
 		})
 	}
@@ -271,7 +272,11 @@ func (f *Farm) Build(ctx context.Context, schedule Schedule, options entities.Bu
 		authfile:      options.Authfile,
 		skipTLSVerify: options.SkipTLSVerify,
 	}
-	manifestListBuilder := newManifestListBuilder(reference, f.localEngine, listBuilderOptions)
+	// Bug #25039: manifestListBuilder now returns an error should a dodgy reference be provided.
+	manifestListBuilder, err := newManifestListBuilder(reference, f.localEngine, listBuilderOptions)
+	if err != nil {
+		return fmt.Errorf("failed to create manifest list %q: %w", reference, err)
+	}
 
 	// Start builds in parallel and wait for them all to finish.
 	var (

pkg/farm/list_builder.go

@@ -10,6 +10,7 @@ import (
 	"github.com/hashicorp/go-multierror"
 	"github.com/sirupsen/logrus"
 	"go.podman.io/image/v5/docker"
+	"go.podman.io/image/v5/docker/reference"
 	"go.podman.io/image/v5/types"
 )
 
@@ -20,20 +21,50 @@ type listBuilderOptions struct {
 	skipTLSVerify *bool
 }
 
+// Bug #25039: listLocal now holds the reference of the manifest to be built
+// as a referenceNamed, rather than as a plain string. It is now made available in two
+// string formats via two member functions (listName() and listReference())
+//
+//	reference  := name [ ":" tag ] [ "@" digest ]
+//	name       := [domain '/'] path-component ['/' path-component]*
+//
+// Prior to this fix, the program was incorrectly using the fuller `reference` format
+// in places that actually required the shorter `name` format, and failing as a consequence.
+// The program only worked when the reference was supplied untagged (i.e some.domain/name).
+// In such situations, `name` and `reference` formats would be identical.
+//
+// In the body of the code, listName has been refactored to listReference() where that is
+// appropriate form to use in that context.
 type listLocal struct {
-	listName    string
+	listRef     reference.Named
 	localEngine entities.ImageEngine
 	options     listBuilderOptions
 }
 
-// newManifestListBuilder returns a manifest list builder which saves a
-// manifest list and images to local storage.
-func newManifestListBuilder(listName string, localEngine entities.ImageEngine, options listBuilderOptions) *listLocal {
+func (l *listLocal) listName() string {
+	// i.e. domain/path-component
+	return l.listRef.Name()
+}
+
+func (l *listLocal) listReference() string {
+	// i.e. domain/path-component:tag
+	return l.listRef.String()
+}
+
+// Bug #25039: manifestListBuilder amended to address problems arising from the
+// provision of an incorrectly specified reference. If the provided reference
+// will not parse correctly, the function now throws an error.
+func newManifestListBuilder(listName string, localEngine entities.ImageEngine, options listBuilderOptions) (*listLocal, error) {
+	ref, err := reference.ParseNamed(listName)
+
+	if err != nil {
+		return nil, fmt.Errorf("could not parse reference %q: %w", listName, err)
+	}
 	return &listLocal{
-		listName:    listName,
+		listRef:     ref,
 		options:     options,
 		localEngine: localEngine,
-	}
+	}, err
 }
 
 // Build retrieves images from the build reports and assembles them into a
@@ -45,15 +76,17 @@ func (l *listLocal) build(ctx context.Context, images map[entities.BuildReport]e
 		skipTLSVerify = types.NewOptionalBool(*l.options.skipTLSVerify)
 	}
 
-	exists, err := l.localEngine.ManifestExists(ctx, l.listName)
+	exists, err := l.localEngine.ManifestExists(ctx, l.listReference())
 	if err != nil {
 		return "", err
 	}
 	// Create list if it doesn't exist
+	//
+	// Bug #25039: we can safely use the longer form, (i.e. potentially tagged), listReference() here. (previously listName)
 	if !exists.Value {
-		_, err = l.localEngine.ManifestCreate(ctx, l.listName, []string{}, entities.ManifestCreateOptions{SkipTLSVerify: skipTLSVerify})
+		_, err = l.localEngine.ManifestCreate(ctx, l.listReference(), []string{}, entities.ManifestCreateOptions{SkipTLSVerify: skipTLSVerify})
 		if err != nil {
-			return "", fmt.Errorf("creating manifest list %q: %w", l.listName, err)
+			return "", fmt.Errorf("creating manifest list %q: %w", l.listReference(), err)
 		}
 	}
 
@@ -67,14 +100,17 @@ func (l *listLocal) build(ctx context.Context, images map[entities.BuildReport]e
 		pushGroup.Go(func() error {
 			logrus.Infof("pushing image %s", image.ID)
 			defer logrus.Infof("pushed image %s", image.ID)
-			// Push the image to the registry
-			report, err := engine.Push(ctx, image.ID, l.listName+docker.UnknownDigestSuffix, entities.ImagePushOptions{Authfile: l.options.authfile, Quiet: false, SkipTLSVerify: skipTLSVerify})
+
+			// Bug #25039: prior to fix, would crash here owing to the UnknownDigestSuffix being inappropriately applied to
+			// the longer `reference` format . We can only use the shorter `name` format here.
+			report, err := engine.Push(ctx, image.ID, l.listName()+docker.UnknownDigestSuffix, entities.ImagePushOptions{Authfile: l.options.authfile, Quiet: false, SkipTLSVerify: skipTLSVerify})
 			if err != nil {
 				return fmt.Errorf("pushing image %q to registry: %w", image, err)
 			}
 			refsMutex.Lock()
 			defer refsMutex.Unlock()
-			refs = append(refs, "docker://"+l.listName+"@"+report.ManifestDigest)
+
+			refs = append(refs, "docker://"+l.listName()+"@"+report.ManifestDigest)
 			return nil
 		})
 	}
@@ -108,18 +144,18 @@ func (l *listLocal) build(ctx context.Context, images map[entities.BuildReport]e
 
 	// Clear the list in the event it already existed
 	if exists.Value {
-		_, err = l.localEngine.ManifestListClear(ctx, l.listName)
+		_, err = l.localEngine.ManifestListClear(ctx, l.listReference())
 		if err != nil {
-			return "", fmt.Errorf("error clearing list %q", l.listName)
+			return "", fmt.Errorf("error clearing list %q", l.listReference())
 		}
 	}
 
 	// Add the images to the list
-	listID, err := l.localEngine.ManifestAdd(ctx, l.listName, refs, entities.ManifestAddOptions{Authfile: l.options.authfile, SkipTLSVerify: skipTLSVerify})
+	listID, err := l.localEngine.ManifestAdd(ctx, l.listReference(), refs, entities.ManifestAddOptions{Authfile: l.options.authfile, SkipTLSVerify: skipTLSVerify})
 	if err != nil {
 		return "", fmt.Errorf("adding images %q to list: %w", refs, err)
 	}
-	_, err = l.localEngine.ManifestPush(ctx, l.listName, l.listName, entities.ImagePushOptions{Authfile: l.options.authfile, SkipTLSVerify: skipTLSVerify})
+	_, err = l.localEngine.ManifestPush(ctx, l.listReference(), l.listReference(), entities.ImagePushOptions{Authfile: l.options.authfile, SkipTLSVerify: skipTLSVerify})
 	if err != nil {
 		return "", err
 	}
@@ -131,5 +167,5 @@ func (l *listLocal) build(ctx context.Context, images map[entities.BuildReport]e
 		}
 	}
 
-	return l.listName, nil
+	return l.listReference(), nil
 }

test/e2e/farm_build_test.go

@@ -141,6 +141,7 @@ var _ = Context("Testing farm build functionality :", Ordered, func() {
 	const OFFLINE_URL = "OFFLINE_URL"
 	const GOOD_SHORT_TAG = "GOOD_SHORT_TAG"
 	const GOOD_LONG_TAG = "GOOD_LONG_TAG"
+	const GOOD_VERY_LONG_TAG = "GOOD_VERY_LONG_TAG"
 
 	type testImageDescriptor struct {
 		image      string
@@ -400,15 +401,6 @@ var _ = Context("Testing farm build functionality :", Ordered, func() {
 		hostArch = strings.Trim(hostArch, "\\\"/")
 
 		// emuInfo = podmanStaticLocal.PodmanExitCleanly("info", "--format", "{{json .Host.EmulatedArchitectures}}").OutputToString()
-
-		// fmt.Printf("socket = %s\n", podmanStaticTest.RemoteSocket)
-		// fmt.Printf("url = %s\n", proxyConnectionURL)
-		// fmt.Printf("offlineUrl = %s\n", offlineConnectionURL)
-		// fmt.Printf("host architecture = %s\n", hostArch)
-		// fmt.Printf("host emulation capability = %s\n", emuInfo)
-		// fmt.Printf("PODMAN_CONNECTIONS_CONF = %s\n", connectionsConf)
-		// fmt.Printf("CONTAINERS_CONF = %s\n", containersConf)
-
 	})
 	/*##########################################################################################################*/
 
@@ -962,6 +954,10 @@ var _ = Context("Testing farm build functionality :", Ordered, func() {
 				scenario.tag = goodTagBase + strings.ToLower(RandomString(10)) + ":tag"
 			}
 
+			if scenario.tag == GOOD_VERY_LONG_TAG {
+				scenario.tag = goodTagBase + strings.ToLower(RandomString(10)) + "/path/path/path:tag"
+			}
+
 			return scenario
 		}
 		/*##########################################################################################################*/
@@ -1216,14 +1212,19 @@ var _ = Context("Testing farm build functionality :", Ordered, func() {
 				},
 			),
 			//
-			// NB: THIS TEST SHOULD WORK, BUT FAILS IN THE CURRENT PRODUCTION CODE
+			Entry("proxyFarm build with full reference form of tag (registry:port/path:tag)",
+				withTestScenarioOf{farm: "proxyFarm", params: "", image: standardTestImage, tag: GOOD_LONG_TAG},
+				expectBuildsOf{
+					build{arch: HOST_ARCH, expectedTobeBuiltOn: LOCAL_HOST, usingEmulation: false, withCleanup: false},
+				},
+			),
 			//
-			// Entry("proxyFarm build with full reference form of tag",
-			// 	withTestScenarioOf{farm: "proxyFarm", params: "", image: standardTestImage, tag: GOOD_LONG_TAG},
-			// 	expectBuildsOf{
-			//  		build{arch: HOST_ARCH, expectedTobeBuiltOn: LOCAL_HOST, usingEmulation: false, withCleanup: false},
-			// 	},
-			// ),
+			Entry("proxyFarm build with even fuller reference form of tag (registry:port/path/path/path:tag)",
+				withTestScenarioOf{farm: "proxyFarm", params: "", image: standardTestImage, tag: GOOD_VERY_LONG_TAG},
+				expectBuildsOf{
+					build{arch: HOST_ARCH, expectedTobeBuiltOn: LOCAL_HOST, usingEmulation: false, withCleanup: false},
+				},
+			),
 			//
 			Entry("proxyFarm build with --local=true",
 				withTestScenarioOf{farm: "proxyFarm", params: "--local=true", image: standardTestImage, tag: GOOD_SHORT_TAG},