Fix pull parameter parsing for compat `/build` endpoint #19591

matejvasek · 2023-08-10T22:09:52Z

Changes

Fixing Docker client incompatibility:

# Broken compatibility with the docker CLI
docker build . -t alpine-1000 --pull
Sending build context to Docker daemon  2.048kB
Error response from daemon: failed to parse query parameter 'pull': "1": invalid pull policy: "1"

Revert compat,build: pull must accept string #18583 since it breaks compatibility with standard docker client.
Previously it was believed the pull parameter is pull-policy since REST API doc says it is string, not bool. However it fact it is a boolean. Confusingly enough Docker indeed accept values like always or never but these values are interpreted as true by Docker. As matter of the fact it appears that there is no such a thing as pull-policy in Docker.
Use custom boolean query param deserializer for all compat endpoints to get closer to Docker's way.
This means anything but "", "0", "no", "false", "none" (ignoring case) is considered to be true.

More context: #17778 (comment)

Correct parsing of the pull query parameter for the compat /build endpoint.

matejvasek · 2023-08-10T22:16:05Z

/cc @Luap99 @flouthoc

matejvasek · 2023-08-10T22:35:22Z

I do not insist on the second commit but we definitely must revert #18583.

matejvasek · 2023-08-10T22:39:48Z

Or if you do not like proliferation of CompatDecoderKey we can use that specific decoder only for the endpoint in question (at least for now).

matejvasek · 2023-08-10T22:42:39Z

/cc @mheon

matejvasek · 2023-08-10T23:47:40Z

wrt DCO: do I really need to sign revert commit?

flouthoc

LGTM

Its weird though to drift away from docs here: https://docs.docker.com/engine/api/v1.42/#tag/Image/operation/ImageBuild ( but since your investigation points that there might be an issue with the docker documentation itself, I am convinced ) lets wait for other maintainers.

This is a breaking change even if this is reverting to an old behavior.

openshift-ci · 2023-08-11T04:44:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, matejvasek

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~~ [flouthoc]

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

vrothberg · 2023-08-11T07:11:16Z

Please add rather verbose explanations on why we are reverting the commit. It has already been released.

Luap99 · 2023-08-11T08:44:26Z

#17778 (comment) sounds reasonable but I agree with @vrothberg, explanations like this should be part of the commit message.

rhatdan · 2023-08-11T08:45:17Z

Is there a test to make sure the libpod entry point still works with the string passing?

matejvasek · 2023-08-11T12:14:40Z

@rhatdan I don't know, but I think I have not touched libpod part at all.

matejvasek · 2023-08-11T12:18:46Z

One questionable thing about this PR is changing boolean parser for all query params of all compat endpoints.

matejvasek · 2023-08-11T13:46:05Z

One questionable thing about this PR is changing boolean parser for all query params of all compat endpoints.

But it changes it in mostly compatible way. Strings "true", "1" are still true. Strings "false", "0" are still false. However string "f" will be now interpreted as true, previously it was false. But this is how Docker converts string to booleans.

matejvasek · 2023-08-11T13:46:56Z

@vrothberg @Luap99 I updated description.

matejvasek · 2023-08-11T14:28:19Z

sending using opt=blah should return a error to the caller and not assume true IMO

@Luap99 Wholeheartedly agree, but tell that to moby.

matejvasek · 2023-08-11T14:29:10Z

Also as I said I am fine with just reverting to previous state where presumably strconv.ParseBool() was used.

matejvasek · 2023-08-11T15:03:48Z

pkg/api/handlers/compat/images_build.go

@@ -580,19 +579,8 @@ func BuildImage(w http.ResponseWriter, r *http.Request) {
 		pullPolicy = buildahDefine.PolicyMap[query.PullPolicy]
 	} else {
 		if _, found := r.URL.Query()["pull"]; found {


@Luap99 Is this found guard needed? Why?

I mean if the pull param is not present then quar.yPull is false anyway right?

Yes I don't think that is needed and should work without it.

matejvasek · 2023-08-11T15:05:25Z

@Luap99 I fixed decoder discrimination for libpod and compat endpoints.

Luap99

LGTM

matejvasek · 2023-08-11T15:44:11Z

Draft of little bit more conservative fix /~https://github.com/containers/podman/pull/19601/files

matejvasek · 2023-08-11T16:01:52Z

@Luap99 @rhatdan IMO it would be good to backport this into patch releases (4.6.2). If this PR seems too big/risky for backporting we can consider something more conservative like #19601

vrothberg · 2023-08-14T11:32:42Z

@vrothberg @Luap99 I updated description.

I don't see it in the commits.

flouthoc · 2023-08-14T11:45:07Z

@matejvasek I think PR description is updated but still it looks less verbose as compared to your investigation here #17778 (comment), I'd vote for including this in commit body or alteast a link to it: #17778 (comment)

This reverts commit 5b148a0. Reverting to treating the `pull` query parameter as a boolean. Because of deceiving Docker API documentation it was assumed that the parameter is pull-policy, however that is not true. Docker does treat `pull` as a boolean. What is interesting is that Docker indeed accepts strings like `always` or `never` however Docekr both of these strings treat as `true`, not as pull-policy. As matter of the fact it seems there is no such a thing as pull-policy in Docker. More context containers#17778 (comment) Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2023-08-14T13:06:25Z

@vrothberg @flouthoc I updated commit message.

In Docker anything but "", "0", "no", "false", "none" (ignoring case) is considered to be true. Signed-off-by: Matej Vasek <mvasek@redhat.com>

rhatdan · 2023-08-14T17:59:40Z

/lgtm

matejvasek · 2023-08-14T18:52:11Z

@rhatdan will this be eligible for 4.6.2?

Luap99 · 2023-08-15T09:07:20Z

I think backporting #19601 makes more sense IMO as this one has less risk involved, if you need the backport in RHEL you need to file a bugzilla as 4.6.1-rhel is already frozen for RHEL.

matejvasek · 2023-08-15T12:54:22Z

What will be time period between 4.6.1-rhel and 4.6.2-rhel ? @Luap99

Luap99 · 2023-08-15T13:01:14Z

There will never be a 4.6.2 in RHEL, RHEL is frozen for each minor RHEL release. Then only approved backports are allowed to the rhel branches so you need to file bugzillas (or Jira bugs soon) for those in order to get this fixed in RHEL.
4.6.1 will go into RHEL 8.9/9.3 and then next version will be only in 6 months for 8.10/9.4 which by then will likely by podman 4.8 or later whatever we decide upstream.

matejvasek · 2023-08-15T13:01:40Z

@Luap99 I absolutely want this fix in rhel.

matejvasek · 2023-08-15T13:02:01Z

#19631

matejvasek · 2023-08-15T13:04:10Z

@Luap99 https://issues.redhat.com or somewhere else?

TomSweeneyRedHat · 2023-08-15T21:05:08Z

@matejvasek I think this is what you're looking for: https://bugzilla.redhat.com/show_bug.cgi?id=2232127

matejvasek · 2023-08-15T21:11:14Z

@TomSweeneyRedHat yeah I created that issue after chat on Slack with @Luap99 .

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Aug 10, 2023

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

openshift-ci bot requested review from flouthoc and Luap99 August 10, 2023 22:16

openshift-ci bot requested a review from mheon August 10, 2023 22:42

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Aug 10, 2023

matejvasek changed the title ~~Fix build pull compat~~ Fix pull parameter parsing for compat /build endpoint Aug 10, 2023

matejvasek force-pushed the fix-build-pull-compat branch from eb56909 to 18fba27 Compare August 11, 2023 00:16

matejvasek marked this pull request as ready for review August 11, 2023 00:17

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2023

matejvasek force-pushed the fix-build-pull-compat branch 3 times, most recently from d31b733 to 7622bfc Compare August 11, 2023 01:39

flouthoc approved these changes Aug 11, 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 Aug 11, 2023

matejvasek commented Aug 11, 2023

View reviewed changes

Luap99 reviewed Aug 11, 2023

View reviewed changes

matejvasek mentioned this pull request Aug 11, 2023

fix: pull param parsing for the /build compat endpoint #19601

Closed

matejvasek force-pushed the fix-build-pull-compat branch from 6120607 to a3f1451 Compare August 14, 2023 13:06

fix: Docker API compatible bool deserialization

f33b01b

In Docker anything but "", "0", "no", "false", "none" (ignoring case) is considered to be true. Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the fix-build-pull-compat branch from a3f1451 to f33b01b Compare August 14, 2023 13:10

openshift-ci bot assigned rhatdan Aug 14, 2023

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

openshift-merge-robot merged commit 824c766 into containers:main Aug 14, 2023

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 Nov 14, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2023

Fix pull parameter parsing for compat /build endpoint #19591

Fix pull parameter parsing for compat /build endpoint #19591

Conversation

matejvasek commented Aug 10, 2023 • edited Loading

Changes

matejvasek commented Aug 10, 2023

matejvasek commented Aug 10, 2023 • edited Loading

matejvasek commented Aug 10, 2023 • edited Loading

matejvasek commented Aug 10, 2023

matejvasek commented Aug 10, 2023

flouthoc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 11, 2023

vrothberg commented Aug 11, 2023

Luap99 commented Aug 11, 2023

rhatdan commented Aug 11, 2023

matejvasek commented Aug 11, 2023

matejvasek commented Aug 11, 2023

matejvasek commented Aug 11, 2023 • edited Loading

matejvasek commented Aug 11, 2023

matejvasek commented Aug 11, 2023

matejvasek commented Aug 11, 2023

matejvasek Aug 11, 2023

Choose a reason for hiding this comment

matejvasek Aug 11, 2023

Choose a reason for hiding this comment

Luap99 Aug 11, 2023

Choose a reason for hiding this comment

matejvasek commented Aug 11, 2023

Luap99 left a comment

Choose a reason for hiding this comment

matejvasek commented Aug 11, 2023 • edited Loading

matejvasek commented Aug 11, 2023 • edited Loading

vrothberg commented Aug 14, 2023

flouthoc commented Aug 14, 2023

matejvasek commented Aug 14, 2023

rhatdan commented Aug 14, 2023

matejvasek commented Aug 14, 2023

Luap99 commented Aug 15, 2023

matejvasek commented Aug 15, 2023

Luap99 commented Aug 15, 2023

matejvasek commented Aug 15, 2023

matejvasek commented Aug 15, 2023

matejvasek commented Aug 15, 2023

TomSweeneyRedHat commented Aug 15, 2023

matejvasek commented Aug 15, 2023

Fix pull parameter parsing for compat `/build` endpoint #19591

Fix pull parameter parsing for compat `/build` endpoint #19591

matejvasek commented Aug 10, 2023 •

edited

Loading

matejvasek commented Aug 10, 2023 •

edited

Loading

matejvasek commented Aug 10, 2023 •

edited

Loading

matejvasek commented Aug 11, 2023 •

edited

Loading

matejvasek commented Aug 11, 2023 •

edited

Loading

matejvasek commented Aug 11, 2023 •

edited

Loading