Resolves #13629 Add RegistryAuthHeader to manifest push #13653

jmontleon · 2022-03-24T20:09:22Z

Signed-off-by: Jason Montleon [email protected]

rhatdan · 2022-03-24T20:20:51Z

You are either going to need to add a test or add the
[NO NEW TESTS NEEDED] to your PR so the test will pass.

rhatdan · 2022-03-25T09:07:24Z

vrothberg · 2022-03-25T09:27:17Z

test/e2e/manifest_test.go

+		lock := GetPortLock("5000")
+		defer lock.Unlock()
+
+		session = podmanTest.Podman([]string{"run", "-d", "-p", "5000:5000", "--name", "registry", "-e", "REGISTRY_AUTH=htpasswd", "-e",


Note to @containers/podman-maintainers:

We should migrate the tests to be using github.com/containers/podman/v4/hack/podman-registry-go: https://github.com/containers/podman/blob/main/pkg/bindings/test/auth_test.go#L10

vrothberg

Changes LGTM, thank you.

manifest add doesn't seem to work yet though.

@jwhonce PTAL

vrothberg · 2022-03-25T09:31:18Z

Could you also squash the two commits into one?

jmontleon · 2022-03-25T15:27:53Z

This now also contains #13651 and the last two steps effectively test it. The last would return zero without the fix @jwmatthews provided.
https://github.com/containers/podman/pull/13653/files#diff-5358dbc9f2e2e54914b369deca4e89d4e3d498e2076dafa5ef49c2079da6f2b1R315-R321

jmontleon · 2022-03-25T15:28:45Z

I squashed the commits and I think I fixed add as well. We'll see what the tests say.

jmontleon · 2022-03-25T16:23:20Z

@vrothberg @rhatdan The add commands are working for me locally, but I had to replace podman on the remote machine/vm because of the changes to the handler. https://github.com/containers/podman/pull/13653/files#diff-044d310f157316ea3de231effda8f6822c587dd263482131fc09b6a2c12b0dda

Is this accounted for in the tests and I've messed something up, or will there need to be an updated image for the tests somehow?

jmontleon · 2022-03-25T18:37:22Z

There's something else going on with the Handlers. It seems like the v4 create handler is going off for every post request. I'm not sure what to make of it; seems like something is very wrong.

jwhonce · 2022-03-25T22:58:37Z

pkg/api/handlers/libpod/manifests.go

@@ -271,7 +290,7 @@ func ManifestPushV3(w http.ResponseWriter, r *http.Request) {
 		utils.Error(w, http.StatusBadRequest, errors.Wrapf(err, "error pushing image %q", query.Destination))
 		return
 	}
-	utils.WriteResponse(w, http.StatusOK, digest)
+	utils.WriteResponse(w, http.StatusOK, handlers.IDResponse{ID: digest})


Good catch! Aligns with swagger now.

jwhonce · 2022-03-25T23:03:49Z

@vrothberg Can the migration to hack/podman-registry-go be a follow-on PR?

jmontleon · 2022-03-26T01:17:24Z

I can have a look and try to adapt it for the PR. Hopefully not too hard now that it's working.

jmontleon · 2022-03-26T04:28:41Z

contrib/cirrus/setup_environment.sh

@@ -36,6 +36,8 @@ do
    fi
 done

+cp hack/podman-registry /bin


Is there a better way to do this?

rhatdan · 2022-03-26T09:24:25Z

/approve

openshift-ci · 2022-03-26T09:24:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmontleon, rhatdan

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

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

rhatdan · 2022-03-26T09:28:11Z

LGTM

mtrmac

(Disclaimer: I’m not very familiar with the HTTP code)

pkg/api/handlers/libpod/manifests.go

mtrmac · 2022-03-26T19:43:12Z

pkg/api/handlers/libpod/manifests.go

+	query.ManifestAddOptions.Authfile = authfile
+	query.ManifestAddOptions.Username = username
+	query.ManifestAddOptions.Password = password
+	query.ManifestAddOptions.All = query.All


This seems unrelated to the description of the PR. Is it intentional?

@mtrmac Sorry for the stupid question, did you mean specifically the last line here query.ManifestAddOptions.All = query.All or all of it

query.ManifestAddOptions.Authfile = authfile query.ManifestAddOptions.Username = username query.ManifestAddOptions.Password = password query.ManifestAddOptions.All = query.All

I'm sorry, I meant just the .All line.

(I’m not saying it’s incorrect, or unwanted, I haven’t checked; it just seems unrelated and I can‘t see any mention of that part.)

It seems unnecessary. I removed it and tests are passing. I was working through getting credentials passed through properly and wasn't sure based on looking at the ImagePushOptions for Push whether this should also be added.

mtrmac · 2022-03-26T19:43:23Z

pkg/api/handlers/libpod/manifests.go

+	body.ManifestAddOptions.Authfile = authfile
+	body.ManifestAddOptions.Username = username
+	body.ManifestAddOptions.Password = password
+	body.ManifestAddOptions.All = body.All


This seems unrelated to the description of the PR. Is it intentional?

Signed-off-by: Jason Montleon <[email protected]>

rhatdan · 2022-03-27T11:12:20Z

/lgtm

jmontleon force-pushed the fix-manifest-push-header branch 2 times, most recently from 460d0c8 to 677f4b1 Compare March 25, 2022 04:15

vrothberg reviewed Mar 25, 2022

View reviewed changes

jmontleon force-pushed the fix-manifest-push-header branch 3 times, most recently from 390dae9 to 3d19293 Compare March 25, 2022 15:24

jmontleon force-pushed the fix-manifest-push-header branch from 3d19293 to f6a1aca Compare March 25, 2022 21:23

jwhonce reviewed Mar 25, 2022

View reviewed changes

jmontleon force-pushed the fix-manifest-push-header branch 3 times, most recently from 222476e to 7a4b861 Compare March 26, 2022 04:27

jmontleon commented Mar 26, 2022

View reviewed changes

contrib/cirrus/setup_environment.sh

@@ -36,6 +36,8 @@ do

fi

done

cp hack/podman-registry /bin

Copy link

Author

jmontleon Mar 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a better way to do this?

jmontleon force-pushed the fix-manifest-push-header branch 2 times, most recently from e675746 to 6d9f72c Compare March 26, 2022 07:04

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2022

mtrmac reviewed Mar 26, 2022

View reviewed changes

Resolves containers#13629 Add RegistryAuthHeader to manifest push

3cc1739

Signed-off-by: Jason Montleon <[email protected]>

jmontleon force-pushed the fix-manifest-push-header branch from 6d9f72c to 3cc1739 Compare March 26, 2022 20:39

openshift-ci bot assigned rhatdan Mar 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2022

openshift-merge-robot merged commit 56b2937 into containers:main Mar 27, 2022

jwmatthews mentioned this pull request Mar 27, 2022

Fixes errors from 'manifest push' being dropped in remote case #13651

Merged

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 Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolves #13629 Add RegistryAuthHeader to manifest push #13653

Resolves #13629 Add RegistryAuthHeader to manifest push #13653

jmontleon commented Mar 24, 2022

rhatdan commented Mar 24, 2022

rhatdan commented Mar 25, 2022

vrothberg Mar 25, 2022

vrothberg left a comment •

edited

Loading

vrothberg commented Mar 25, 2022

jmontleon commented Mar 25, 2022

jmontleon commented Mar 25, 2022

jmontleon commented Mar 25, 2022 •

edited

Loading

jmontleon commented Mar 25, 2022

jwhonce Mar 25, 2022 •

edited

Loading

jwhonce commented Mar 25, 2022

jmontleon commented Mar 26, 2022

jmontleon Mar 26, 2022

rhatdan commented Mar 26, 2022

openshift-ci bot commented Mar 26, 2022

rhatdan commented Mar 26, 2022

mtrmac left a comment

mtrmac Mar 26, 2022

jmontleon Mar 26, 2022

mtrmac Mar 26, 2022

jmontleon Mar 26, 2022

mtrmac Mar 26, 2022

rhatdan commented Mar 27, 2022

Resolves #13629 Add RegistryAuthHeader to manifest push #13653

Resolves #13629 Add RegistryAuthHeader to manifest push #13653

Conversation

jmontleon commented Mar 24, 2022

rhatdan commented Mar 24, 2022

rhatdan commented Mar 25, 2022

vrothberg Mar 25, 2022

Choose a reason for hiding this comment

vrothberg left a comment • edited Loading

Choose a reason for hiding this comment

vrothberg commented Mar 25, 2022

jmontleon commented Mar 25, 2022

jmontleon commented Mar 25, 2022

jmontleon commented Mar 25, 2022 • edited Loading

jmontleon commented Mar 25, 2022

jwhonce Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

jwhonce commented Mar 25, 2022

jmontleon commented Mar 26, 2022

jmontleon Mar 26, 2022

Choose a reason for hiding this comment

rhatdan commented Mar 26, 2022

openshift-ci bot commented Mar 26, 2022

rhatdan commented Mar 26, 2022

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Mar 26, 2022

Choose a reason for hiding this comment

jmontleon Mar 26, 2022

Choose a reason for hiding this comment

mtrmac Mar 26, 2022

Choose a reason for hiding this comment

jmontleon Mar 26, 2022

Choose a reason for hiding this comment

mtrmac Mar 26, 2022

Choose a reason for hiding this comment

rhatdan commented Mar 27, 2022

vrothberg left a comment •

edited

Loading

jmontleon commented Mar 25, 2022 •

edited

Loading

jwhonce Mar 25, 2022 •

edited

Loading