Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow save image remove-signatures #7956

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Allow save image remove-signatures #7956

merged 1 commit into from
Oct 22, 2020

Conversation

QiWang19
Copy link
Contributor

@QiWang19 QiWang19 commented Oct 7, 2020 •
edited
Loading

remove signatures in podman save since the image formats do not support signatures
Close: #7659

Signed-off-by: Qi Wang [email protected]

@TomSweeneyRedHat
Copy link
Member

Tests and bash completions?

@QiWang19
Copy link
Contributor Author

QiWang19 commented Oct 7, 2020

bash completions added. I am figuring out how to test this.

jwhonce
jwhonce requested changes Oct 7, 2020
View reviewed changes
Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

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

@QiWang19 You have new option coded as available to remote users but not coded in the tunnel or bindings packages

@QiWang19 QiWang19 force-pushed the save-rm-sig branch 8 times, most recently from 712d577 to 6bd899f Compare October 14, 2020 01:18
@QiWang19
Copy link
Contributor Author

@jwhonce PTAL

@QiWang19 QiWang19 force-pushed the save-rm-sig branch 6 times, most recently from 15ef3ea to 2323246 Compare October 14, 2020 22:00
rhatdan
rhatdan approved these changes Oct 15, 2020
View reviewed changes
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

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

When would a user ever specify --remove-signatures=false?

test/e2e/save_test.go Outdated
@@ -116,6 +120,71 @@ var _ = Describe("Podman save", func() {
Expect(save).To(ExitWithError())
})

It("podman save remove signature", func() {
SkipIfRemote("podman-remote doesn't suppport tls-verify option")
Copy link
Member

Choose a reason for hiding this comment

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

Podman remote now supports tls-verify.

Copy link
Contributor Author

@QiWang19 QiWang19 Oct 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

podman remove push --tls-verify works, but still see unknown flag: --tls-verify with podman pull,
in the test int remote ubuntu-19 root host https://api.cirrus-ci.com/v1/task/4566543570829312/logs/main.log

[+2546s] Running: /var/tmp/go/src/github.com/containers/podman/bin/podman-remote --remote --url unix:/run/podman/podman-1695fe65d29f17c37c0cd8754dbdd796e29036e2adea66ec67ab760001892f06.sock --remote --url unix:/run/podman/podman-1695fe65d29f17c37c0cd8754dbdd796e29036e2adea66ec67ab760001892f06.sock pull --tls-verify=false --signature-policy=sign/policy.json localhost:5000/alpine
[+2546s] Error: unknown flag: --tls-verify

Copy link
Member

Choose a reason for hiding this comment

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

I opened a PR to fix this.

#8042

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2020
@QiWang19
Copy link
Contributor Author

When would a user ever specify --remove-signatures=false?

The user will use --remove-signatures=false, current image format doesn't support pushing signatures to it.

@QiWang19 QiWang19 force-pushed the save-rm-sig branch 2 times, most recently from 526aa7b to ffe11ea Compare October 15, 2020 18:21
@rhatdan
Copy link
Member

rhatdan commented Oct 18, 2020

@QiWang19 Shouldn't the code do this automatically, IE Does the code know which image stores support signatures? Would the user just get an error telling them to remove signatures and now they need to run the command again with the flag? Does not seem like a good UI experience?

@QiWang19
Copy link
Contributor Author

On the suggestion under the issue #7659 (comment), the remove-signatures defaults to true and avoids the user from getting the error.

@rhatdan
Copy link
Member

rhatdan commented Oct 20, 2020

@QiWang19 Sure, but under what circumstance would the user ever set this to False. When would the user not want to remove-signatures? Bottom line is there a situation where a user would ever type in
--remove-signatures=false or true?
If we want signatures to be pushed to a registry?

@QiWang19
Copy link
Contributor Author

If we want signatures to be pushed to a registry, podman push also has remove-signatures defaults to false. This patch only applies to podman save, current format (oci-archive, oci-dir, docker-dir) doesn't support save with signatures.

@QiWang19 QiWang19 force-pushed the save-rm-sig branch 2 times, most recently from 22f4a48 to bef019b Compare October 20, 2020 20:00
@rhatdan
Copy link
Member

rhatdan commented Oct 21, 2020

When would I ever do a
podman save --remove-signatures=false?

@QiWang19
Copy link
Contributor Author

When would I ever do a
podman save --remove-signatures=false?

Remove the option and remove the signature in the code, the current image format doesn't support saving signatures.

@QiWang19
save image remove signatures
b898f91 
remove signatures to podman save since the image formats do not support signatures
Close: containers#7659

Signed-off-by: Qi Wang <[email protected]>
@rhatdan
Copy link
Member

rhatdan commented Oct 22, 2020

LGTM

@TomSweeneyRedHat
Copy link
Member

LGTM
Would like a head nod from @jwhonce before merging as he'd asked for a change.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce, QiWang19, 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:

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

@jwhonce
Copy link
Member

jwhonce commented Oct 22, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2020
@openshift-merge-robot openshift-merge-robot merged commit 513c261 into containers:master Oct 22, 2020
@fgiloux fgiloux mentioned this pull request Sep 21, 2021
Closed
@vrothberg vrothberg mentioned this pull request Sep 23, 2021
Closed
@github-actions 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 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@rhatdan rhatdan rhatdan approved these changes

@jwhonce jwhonce jwhonce approved these changes

Assignees

@jwhonce jwhonce

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

podman save capability to remove signatures
6 participants
@QiWang19 @TomSweeneyRedHat @rhatdan @openshift-ci-robot @jwhonce @openshift-merge-robot