images/create: handle tag/digest if Tag specified #24184

inknos · 2024-10-07T10:06:37Z

Podman handles /images/create better when fromImage and Tag are specified. Now the tag/digest value provided in Tag will replace the one in fromImage

Fixes: #23938

Does this PR introduce a user-facing change?

None

openshift-ci · 2024-10-07T10:06:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: inknos
Once this PR has been reviewed and has the lgtm label, please assign luap99 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2024-10-07T10:24:37Z

pkg/api/handlers/compat/images.go

+	// if both name and tagOrDigest are defined we try to use the tagOrDigest image part and override the tag/digest
+	if len(name) > 0 && len(tagOrDigest) > 0 && strings.Contains(name, separator) {
+		parts := strings.Split(name, separator)
+		return fmt.Sprintf("%s%s%s", strings.Join(parts[:len(parts)-1], ""), separator, tagOrDigest)
+	}


checking len(tagOrDigest) > 0 is unnecessary as the function returns early if that is empty so we know it cannot be empty here

Then using string.Contains() and strings.Split() adds more overhead then needed here. You can use
strings.LastIndex(name, separator) once and then when the index is > 0 you can trim the name with name[:index]

However more important speaking string parsing registry references like this does not look correct to me. In particular this will fail when a registry host includes a port number, i.e. 127.0.0.1:5000/myimage as this would cause the code to cut at the wrong place. I am not super familiar with c/image but assume we should have some "proper" parsing code there to deal with that already so maybe we could resuse that?
cc @mtrmac

If there is no such code we can use then we must at least ensure the : is after the first /

right, I'll fix it

Yes, things like this should definitely use c/image/v5/docker/reference; around here, before pkg/shortnames / PodmanOnlyShortNamesIgnoreRegistriesConfAndForceDockerHub is used, probably .Parse, but very carefully — have unit tests for busybox, docker.io/busybox, docker.io/library/busybox.

(As a general principle, I think strings should be parsed into Go values as close to the surface of the app as possible, and then only turned back into strings for I/O or interaction with other processes. But it seems that would be a large restructuring — and we don’t have distinct Go types for “possibly a short name” vs. “a name which certainly contains a host name”. And, anyway, Go makes it very tedious to write bulletproof encapsulation.)

Compare e.g. https://github.com/containers/common/blob/acfcaabd70744222dfcaf962850531c640774d67/libimage/normalize.go#L105 for a very rough shape of things — parse, type check, use TrimNamed/WithTag/WithDigest.

(For the record, I didn’t investigate the motivating API request / behavior at this point.)

would url parsing work ?

No no no no no :)

That would introduce a lot of syntax (like % escapes, and / vs // vs ///) that we actively don’t want.

127.0.0.1:5000/myimage is a valid image reference (i.e. not tag default to latest). In this case you would join the tag at the wrong place for name=127.0.0.1:5000/myimage and tagOrDigest=mytag will result in a broken 127.0.0.1:mytag AFAICS

I think it would be really good to add unit tests for this function to check these corner cases properly

yup, to be honest, I would split this functionality into a separate function and unit test that

I mean you could test the entire mergeNameAndTagOrDigest(), I don't think it needs to be split smaller than that but I would not be opposed either

I really strongly want this to use the standard c/image/docker/reference parser and not custom code.

E.g. consider that we’ve had a request for IPv6 addresses (with their embedded :s) for years, and for all I know, it might actually happen some day.

Podman handles /images/create better when fromImage and Tag are specified. Now the tag/digest value provided in Tag will replace the one in fromImage Fixes: containers#23938 Signed-off-by: Nicola Sella <[email protected]>

Luap99 · 2024-10-09T16:16:37Z

pkg/api/handlers/compat/images.go

@@ -45,6 +45,11 @@ func mergeNameAndTagOrDigest(name, tagOrDigest string) string {
 		// We have a digest, so let's change the separator.
 		separator = "@"
 	}
+	// if both name and tagOrDigest are defined we try to use the tagOrDigest image part and override the tag/digest
+	if len(name) > 0 && strings.Contains(name, separator) {


the contains is unnecessary, LastIndex already tells you if it contains or not so we only need to walk the string once not twice

mtrmac

Oops, the review was left as pending comments. Sorry about that.

mtrmac · 2024-10-07T16:53:52Z

pkg/api/handlers/compat/images.go

@@ -45,6 +45,12 @@ func mergeNameAndTagOrDigest(name, tagOrDigest string) string {
 		// We have a digest, so let's change the separator.
 		separator = "@"
 	}
+
+	// if both name and tagOrDigest are defined we try to use the tagOrDigest image part and override the tag/digest
+	if len(name) > 0 && len(tagOrDigest) > 0 && strings.Contains(name, separator) {


What happens if name contains a tag and tagOrDigest contains a digest, or vice versa?

mtrmac · 2024-10-07T17:00:32Z

pkg/api/handlers/compat/images.go

+	// if both name and tagOrDigest are defined we try to use the tagOrDigest image part and override the tag/digest
+	if len(name) > 0 && len(tagOrDigest) > 0 && strings.Contains(name, separator) {
+		parts := strings.Split(name, separator)
+		return fmt.Sprintf("%s%s%s", strings.Join(parts[:len(parts)-1], ""), separator, tagOrDigest)
+	}


Yes, things like this should definitely use c/image/v5/docker/reference; around here, before pkg/shortnames / PodmanOnlyShortNamesIgnoreRegistriesConfAndForceDockerHub is used, probably .Parse, but very carefully — have unit tests for busybox, docker.io/busybox, docker.io/library/busybox.

(As a general principle, I think strings should be parsed into Go values as close to the surface of the app as possible, and then only turned back into strings for I/O or interaction with other processes. But it seems that would be a large restructuring — and we don’t have distinct Go types for “possibly a short name” vs. “a name which certainly contains a host name”. And, anyway, Go makes it very tedious to write bulletproof encapsulation.)

Compare e.g. https://github.com/containers/common/blob/acfcaabd70744222dfcaf962850531c640774d67/libimage/normalize.go#L105 for a very rough shape of things — parse, type check, use TrimNamed/WithTag/WithDigest.

(For the record, I didn’t investigate the motivating API request / behavior at this point.)

mtrmac · 2024-10-08T18:24:12Z

pkg/api/handlers/compat/images.go

+	// if both name and tagOrDigest are defined we try to use the tagOrDigest image part and override the tag/digest
+	if len(name) > 0 && len(tagOrDigest) > 0 && strings.Contains(name, separator) {
+		parts := strings.Split(name, separator)
+		return fmt.Sprintf("%s%s%s", strings.Join(parts[:len(parts)-1], ""), separator, tagOrDigest)
+	}


No no no no no :)

That would introduce a lot of syntax (like % escapes, and / vs // vs ///) that we actively don’t want.

github-actions · 2024-11-09T00:08:47Z

A friendly reminder that this PR had no activity for 30 days.

openshift-ci bot added the release-note-none label Oct 7, 2024

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 7, 2024

Luap99 reviewed Oct 7, 2024

View reviewed changes

inknos force-pushed the issue-23938 branch from 533be65 to dd5c87e Compare October 9, 2024 13:49

images/create: handle tag/digest if Tag specified

1778a90

Podman handles /images/create better when fromImage and Tag are specified. Now the tag/digest value provided in Tag will replace the one in fromImage Fixes: containers#23938 Signed-off-by: Nicola Sella <[email protected]>

inknos force-pushed the issue-23938 branch from dd5c87e to 1778a90 Compare October 9, 2024 13:50

Luap99 reviewed Oct 9, 2024

View reviewed changes

mtrmac reviewed Oct 9, 2024

View reviewed changes

github-actions bot added the stale-pr label Nov 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

images/create: handle tag/digest if Tag specified #24184

images/create: handle tag/digest if Tag specified #24184

inknos commented Oct 7, 2024

openshift-ci bot commented Oct 7, 2024

Luap99 Oct 7, 2024

inknos Oct 7, 2024

mtrmac Oct 7, 2024

baude Oct 8, 2024

mtrmac Oct 8, 2024

Luap99 Oct 9, 2024

inknos Oct 9, 2024

Luap99 Oct 9, 2024

mtrmac Oct 9, 2024

mtrmac Oct 9, 2024

Luap99 Oct 9, 2024

mtrmac left a comment

mtrmac Oct 7, 2024

mtrmac Oct 7, 2024

mtrmac Oct 8, 2024

github-actions bot commented Nov 9, 2024

images/create: handle tag/digest if Tag specified #24184

Are you sure you want to change the base?

images/create: handle tag/digest if Tag specified #24184

Conversation

inknos commented Oct 7, 2024

Does this PR introduce a user-facing change?

openshift-ci bot commented Oct 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2024