trust: switch policy.json lookup to configfile#28508
Conversation
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
3 similar comments
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
Luap99
left a comment
There was a problem hiding this comment.
some smaller comments, overall this seems mostly fine.
Longer term we really need to de duplicate this logic out of podman IMO
|
(I have added “No new tests” to let the CI run, and switched the PR to draft, correspondingly. If this PR does end up making changes to the repo, please consider removing the label.) |
| if err != nil { | ||
| logrus.Warnf("could not resolve relative path to binary: %q", err) | ||
| } | ||
| paths = append(paths, filepath.Join(filepath.Dir(p), DefaultPolicyJSONPath, policyfile)) |
There was a problem hiding this comment.
Note this part, this seems to allow shipping Podman machine in a non-default / relocatable location.
Do we know that this is no longer necessary?
There was a problem hiding this comment.
this has never been used or documented by us, it comes from patches before we agreed to not mandate policy.json for podman machine.
Using the officially documented locations consistently seem much much saner to me
There was a problem hiding this comment.
OTOH we have this week been talking about starting to sign the machine images, so something like this might be coming.
If we are not worried about breaking users, I’m fine with switching back to the default now and perhaps changing the behavior again later.
Luap99
left a comment
There was a problem hiding this comment.
ack overall, just some minor stuff and the missing docs.
mtrmac
left a comment
There was a problem hiding this comment.
ACK overall, I didn’t carefully look at the CLI integration.
I’ll also drop the “no new tests” label now.
9669567 to
53a5d13
Compare
Use shared configfile instead of custom policy.json path handling. This updates ocipull to rely on signature.DefaultPolicy(), removes explicit SignaturePolicyPath, and replaces trust's custom default-policy path logic with common configfile code. Replace hidden `--policypath` with --signature-policy` and require it for `trust set` command instead of path resolution based on configfile. For `trust get`, the `--signature-policy` is optional. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
|
I rebased it to main and also to latest container-libs. |
|
/packit test |
Luap99
left a comment
There was a problem hiding this comment.
changes LGTM overall, I would squash commit 1 and 2 though
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
mtrmac
left a comment
There was a problem hiding this comment.
LGTM if tests pass. Thanks!
|
/packit test |
|
I'm down to revdeps test, which fails with suspicious message: |
|
But I see the same test failure also on other recent PRs (#28550), so maybe it's just tft issue. I will retry this later today. |
Luap99
left a comment
There was a problem hiding this comment.
LGTM, packit tests are non blocking and can be ignored, the tmt reverse dep tests use the wrong netavark version.
Use shared configfile instead of custom policy.json path handling. This switches ocipull to signature.DefaultPolicy(), removes SignaturePolicyPath plumbing, and updates podman image trust to use
--signature-policy.Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?