fix: pull error response docker rest api compatibility #20239

jackgris · 2023-10-03T03:05:32Z

This is related to the issue #20013

I have not added another e2e test, because in the pull_test.go file the test called "podman pull bogus image" is checking that the response contains "unauthorized: access to the requested resource is not authorized"

One thing that I was not sure you agreed with is the modified jsonmessage.JSONMessage{} type. That is used there but belongs to the Docker jsonmessage package inside the vendor folder. Maybe it is better to copy that type into the same package and modify it there.

Does this PR introduce a user-facing change?

This changes the error response message to the comment in the issue.

openshift-ci · 2023-10-03T03:05:37Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vrothberg

Thanks for contributing!

I don't think that's correct. Docker calls FormatStatus which in turn does exactly what Podman seems to be doing already, see
https://github.com/moby/moby/blob/master/pkg/streamformatter/streamformatter.go#L32-L41.

So my current conclusion is that the Docker API docs are incomplete?

jackgris · 2023-10-03T22:54:42Z

Yes, that can be true. I haven't thought about that before.

vrothberg · 2023-10-04T07:13:53Z

As consequence, clients like the official python library for docker doesn't work because the response has a different schema [...]

You mentioned that in the issue. Do you have a reproducer for that?

jackgris · 2023-10-04T21:43:46Z

Sorry, I didn't clarify, but I am not the creator of the issue. I never used that library the owner of that issue is @Alex-Izquierdo, I just saw what he mentioned and reproduced the messages using the Docker and Podman API.
If I run these commands:

curl --no-buffer -XPOST  --unix-socket  ./docker.sock "http:/v1.43/images/create?fromImage=quay.io/idonotexist/idonotexist:dummy"

I received this response:

{"message":"unauthorized: access to the requested resource is not authorized"}

But, If I run Podman:

./bin/podman system service -t 0 --log-level=debug

When I send a request to Podman in the same way:

curl --no-buffer -XPOST  --unix-socket  /run/user/1000/podman/podman.sock "http:/v1.43/images/create?fromImage=quay.io/idonotexist/idonotexist:dummy" | json_pp

I received this response:

{
   "error" : "initializing source docker://quay.io/idonotexist/idonotexist:dummy: reading manifest dummy in quay.io/idonotexist/idonotexist: unauthorized: access to the requested resource is not authorized",
   "errorDetail" : {
      "message" : "initializing source docker://quay.io/idonotexist/idonotexist:dummy: reading manifest dummy in quay.io/idonotexist/idonotexist: unauthorized: access to the requested resource is not authorized"
   },
   "progressDetail" : {}
}

Alex says that the different responses give him trouble when he tries to use those libraries.

vrothberg · 2023-10-05T08:13:14Z

Thanks, @jackgris !

I will take a closer look.

vrothberg · 2023-10-05T09:02:05Z

OK, I believe to see the code in question (see https://github.com/moby/moby/blob/master/api/server/router/image/image_routes.go#L148-L151). Docker will send the error-detail as is if (and only if) no progress data has been sent yet. So Podman's compat API should do the same.

@jackgris do you feel comfortable tackling that or shall we take over?

jackgris · 2023-10-06T03:11:42Z

OK, I'll try to send a new pull request these days.

I'll remove the field that I added to jsonmessage.JSONMessage struct. I think I can return a message of type jsonmessage.JSONError (this type will return the json message only with a message field) or jsonmessage.JSONMessage depending on whether I received reports of some progress before or not.

vrothberg · 2023-10-06T07:18:21Z

Thank you, @jackgris ! I can help writing tests once the PR has been updated.

Alex-Izquierdo · 2023-10-06T10:44:36Z

I tried to take a look, but I realized it wasn't going to be as quick as I had hoped and I haven't had the time to revisit it in detail.

Thanks a lot @jackgris to address the issue.

packit-as-a-service · 2023-10-06T21:47:25Z

Ephemeral COPR build failed. @containers/packit-build please check.

jackgris · 2023-10-06T22:51:41Z

Sorry, I made a mistake and completely forgot to squash my commit, before pushing the change.
Now that you can see what kind of change I was thinking, before modifying anything else, Valentin you were thinking of something different? Or another way to check progress?

vrothberg

The direction looks good to me. Thanks!

vrothberg · 2023-10-09T08:12:04Z

pkg/api/handlers/compat/images.go

+				message := jsonmessage.JSONError{
+					Message: msg,
+				}
+				if err := enc.Encode(message); err != nil {


We can do enc.Encode(report.Error) directly I think.

If I encode the message with enc.Encode(report.Error) Podman will return this message:

{ "message" : "initializing source docker://quay.io/idonotexist/idonotexist:dummy: reading manifest dummy in quay.io/idonotexist/idonotexist: unauthorized: access to the requested resource is not authorized" }

Have more details that the return message unwrapped, that is like this:

{ "message" : "unauthorized: access to the requested resource is not authorized" }

Docker returns the same message as the second option. What is your opinion on this? What do you prefer?

Nice catch! I do not feel strongly about it. However, some libraries try to string-match errors, so returning the same error as Docker is probably the right thing do to.

vrothberg · 2023-10-09T08:12:26Z

pkg/api/handlers/compat/images.go

-			if err := enc.Encode(report); err != nil {
-				logrus.Warnf("Failed to json encode error %q", err.Error())
+
+			if err != nil && !progressSent {


Please add a comment here elaborating on this specific case and why it needs to be done (docker compat).

jackgris · 2023-10-10T01:22:10Z

OK, I think tomorrow I will send a new pull request with the changes you suggest.

vrothberg · 2023-10-11T09:28:43Z

Heads up: in case #20328 is merged before, you need to rebase. It moves the code to another file.

jackgris · 2023-10-11T20:26:04Z

OK, I see that. Now that code is in pkg/api/handlers/utils/images.go.
The only detail is, what do you think about this:
#20239 (comment)

vrothberg

Thanks!

Last step is to add tests which live here: https://github.com/containers/podman/blob/main/test/apiv2/10-images.at

I think we should match against the JSON payload and look for the message field. The syntax of the tests is explained here: https://github.com/containers/podman/tree/main/test/apiv2#writing-tests

jackgris · 2023-10-12T22:57:51Z

Okay, I will follow those steps to write tests. I've never used that tool before; it seems similar to shell scripting.

jackgris · 2023-10-13T23:18:14Z

What do you think about that test? Do you believe it's correct, or does it need to be made more generic?

One thing I find a bit challenging to understand is how to incorporate regular expressions into it. How can I rewrite the test with a regular expression to accept any message value and only check if the 'message' key is present?
I tried different options, but when I try to match that with a regular expression, the test always fails.

This is related to the issue containers#20013 Signed-off-by: Gabriel Pozo <[email protected]>

vrothberg

LGTM

openshift-ci · 2023-10-16T07:36:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackgris, vrothberg

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrothberg]

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

vrothberg · 2023-10-16T07:38:11Z

One thing I find a bit challenging to understand is how to incorporate regular expressions into it. How can I rewrite the test with a regular expression to accept any message value and only check if the 'message' key is present?
I tried different options, but when I try to match that with a regular expression, the test always fails.

Can you give an example? Something like ".* foo .*" should work. I am not sure whether full regular expressions work.

@edsantiago PTAL

edsantiago · 2023-10-16T11:52:53Z

I tried different options, but when I try to match that with a regular expression, the test always fails.

POSIX regexes should work, per the bash [[ operator. Can you give an example of what you tried, and include the exact errors you saw?

jackgris · 2023-10-16T16:07:25Z

First of all, thank you, both.

Well, I think I've found what I was looking for—something like this:

t POST "/images/create?fromImage=quay.io/idonotexist/idonotexist:dummy" 401 \
  .message~'unauthorized: [a-z]'

That test passed. What do you think is better for this case? Do you prefer the way a pull is requested or this new one?

vrothberg · 2023-10-17T07:54:07Z

That test passed. What do you think is better for this case? Do you prefer the way a pull is requested or this new one?

I like the current implementation of the test as it checks for an explicit error message.

LGTM

jackgris · 2023-10-17T15:49:41Z

OK, nice. Is there something more to do to finish the process?

rhatdan · 2023-10-17T18:48:10Z

/lgtm
Thanks @jackgris

vrothberg · 2023-10-18T07:52:51Z

OK, nice. Is there something more to do to finish the process?

Just a LGTM from another maintainer. Thanks a lot for contributing! This fix will make it into the next Podman 4.8 release which is scheduled for November.

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Oct 3, 2023

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

vrothberg reviewed Oct 3, 2023

View reviewed changes

vrothberg approved these changes Oct 9, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2023

jackgris force-pushed the docker-api-error-response branch from c888050 to b70dfb2 Compare October 11, 2023 03:10

jackgris force-pushed the docker-api-error-response branch from 3062836 to e434e6a Compare October 11, 2023 21:50

vrothberg reviewed Oct 12, 2023

View reviewed changes

fix: pull error response docker rest api compatibility

b9f2c4d

This is related to the issue containers#20013 Signed-off-by: Gabriel Pozo <[email protected]>

jackgris force-pushed the docker-api-error-response branch from 694c7c5 to b9f2c4d Compare October 13, 2023 23:20

vrothberg approved these changes Oct 16, 2023

View reviewed changes

openshift-ci bot assigned rhatdan Oct 17, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 17, 2023

rhatdan removed the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Oct 17, 2023

openshift-ci bot merged commit c909afb into containers:main Oct 17, 2023
98 checks passed

jackgris mentioned this pull request Nov 5, 2023

docker rest api compatibility not honored for error responses. #20013

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 17, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pull error response docker rest api compatibility #20239

fix: pull error response docker rest api compatibility #20239

jackgris commented Oct 3, 2023

openshift-ci bot commented Oct 3, 2023

vrothberg left a comment

jackgris commented Oct 3, 2023

vrothberg commented Oct 4, 2023

jackgris commented Oct 4, 2023 •

edited

Loading

vrothberg commented Oct 5, 2023

vrothberg commented Oct 5, 2023

jackgris commented Oct 6, 2023

vrothberg commented Oct 6, 2023

Alex-Izquierdo commented Oct 6, 2023

packit-as-a-service bot commented Oct 6, 2023

jackgris commented Oct 6, 2023

vrothberg left a comment

vrothberg Oct 9, 2023

jackgris Oct 11, 2023

vrothberg Oct 12, 2023

vrothberg Oct 9, 2023

jackgris commented Oct 10, 2023

vrothberg commented Oct 11, 2023

jackgris commented Oct 11, 2023

vrothberg left a comment

jackgris commented Oct 12, 2023

jackgris commented Oct 13, 2023

vrothberg left a comment

openshift-ci bot commented Oct 16, 2023

vrothberg commented Oct 16, 2023

edsantiago commented Oct 16, 2023

jackgris commented Oct 16, 2023 •

edited

Loading

vrothberg commented Oct 17, 2023

jackgris commented Oct 17, 2023

rhatdan commented Oct 17, 2023

vrothberg commented Oct 18, 2023

fix: pull error response docker rest api compatibility #20239

fix: pull error response docker rest api compatibility #20239

Conversation

jackgris commented Oct 3, 2023

Does this PR introduce a user-facing change?

openshift-ci bot commented Oct 3, 2023

vrothberg left a comment

Choose a reason for hiding this comment

jackgris commented Oct 3, 2023

vrothberg commented Oct 4, 2023

jackgris commented Oct 4, 2023 • edited Loading

vrothberg commented Oct 5, 2023

vrothberg commented Oct 5, 2023

jackgris commented Oct 6, 2023

vrothberg commented Oct 6, 2023

Alex-Izquierdo commented Oct 6, 2023

packit-as-a-service bot commented Oct 6, 2023

jackgris commented Oct 6, 2023

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Oct 9, 2023

Choose a reason for hiding this comment

jackgris Oct 11, 2023

Choose a reason for hiding this comment

vrothberg Oct 12, 2023

Choose a reason for hiding this comment

vrothberg Oct 9, 2023

Choose a reason for hiding this comment

jackgris commented Oct 10, 2023

vrothberg commented Oct 11, 2023

jackgris commented Oct 11, 2023

vrothberg left a comment

Choose a reason for hiding this comment

jackgris commented Oct 12, 2023

jackgris commented Oct 13, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 16, 2023

vrothberg commented Oct 16, 2023

edsantiago commented Oct 16, 2023

jackgris commented Oct 16, 2023 • edited Loading

vrothberg commented Oct 17, 2023

jackgris commented Oct 17, 2023

rhatdan commented Oct 17, 2023

vrothberg commented Oct 18, 2023

jackgris commented Oct 4, 2023 •

edited

Loading

jackgris commented Oct 16, 2023 •

edited

Loading