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.

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

Export BuiltinDefaultPolicyPath #2562

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Sep 6, 2024

This field is duplicated in containers/common/pkg/config and is being vendored into bindings, just because it is not exported from containers/image.

This field is duplicated in containers/common/pkg/config and is
being vendored into bindings, just because it is not exported
from containers/image.

Signed-off-by: Daniel J Walsh <[email protected]>
@rhatdan
Copy link
Member Author

rhatdan commented Sep 6, 2024

@mtrmac @Luap99 PTAL

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

NAK; if the goal is to handle #1726 , this is going in the wrong direction, motivating users even more to hard-code specifics of the logic (in this case, exporting the system-wide path motivates users to reimplement the per-user path).

If c/common intends to use the c/image default, it should not be setting the path in SystemContext at all, and the whole need for this goes away. (That can’t immediately happen because the two implementations have already diverged. Also maybe c/image needs some special knowledge of rootless operations .)


There is pkg/trust, which writes to the config files, and that one really needs to know a path, not a policy accessor. So c/image should probably export something — but it wouldn’t be systemDefaultPolicyPath; it would be defaultPolicyPath or maybe a separate “system-wide primary policy config path” / “per-user primary config path”.

@rhatdan
Copy link
Member Author

rhatdan commented Sep 9, 2024

Well how do we move forward on that then?

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 9, 2024

I don’t immediately know; the way the code has already diverged, maybe there is some neat elegant solution which is compatible enough, maybe we will have to live with a gross hack forever. Someone needs to figure that out.


For the purpose of trimming containers/podman#23857 (comment) , the bindings should not be including the whole implementation in pkg/trust in the first place; AFAICS they just need the trust.Policy data type. That can either be split into a subpackage; or the domain/entities/types subpackage could define a separate RPC-only type, separate from the implementation type; or maybe pkg/trust should be importing the type from domain/entities/types — I don’t know what is the general Podman approach to RPC-only types duplicated with implementation types, vs. sharing the two, and where the definitions live.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants