-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
pkg/machine: add custom policy.json logic #21765
pkg/machine: add custom policy.json logic #21765
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99 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 |
bb29f58
to
9fd1831
Compare
So the macos installer change is completely untested and have no idea about the changes there really and just dud what I think seems logical so please someone with a mac test that! |
@n1hility I was unable to figure out how to add this file to the windows installer, can you help me here? We can build podman with |
Sure. If you get the file into the podman-remote zip tasks which are in the main Makefile, that’s the first step. The installer build pulls from those. It likely needs a small update which I can sort out after it’s in those builds. |
9fd1831
to
e40f43f
Compare
OK I think I added it now but I really do not know how to set MACHINE_POLICY_JSON_DIR there? This target seems to be used for other OS'es as well. I have hard time understanding what is and isn't used in the Makefile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building the pkginstaller fails with
CGO_ENABLED=0 GOOS=darwin GOARCH=arm64 go build \
\
-ldflags '-X github.com/containers/podman/v5/libpod/define.gitCommit=e40f43f75b0c3352df5e87a66934a773e9cc06ca -X github.com/containers/podman/v5/libpod/define.buildInfo=1708530478 -X github.com/containers/podman/v5/libpod/config._installPrefix=/usr/local -X github.com/containers/podman/v5/libpod/config._etcDir=/etc -X github.com/containers/podman/v5/pkg/systemd/quadlet._binDir=/usr/local/bin -X github.com/containers/podman/v5/pkg/machine/ocipull.DefaultPolicyJSONPath= -X github.com/containers/common/pkg/config.additionalHelperBinariesDir=/opt/podman/bin MACHINE_POLICY_JSON_DIR=/opt/podman/config ' \
-tags "remote exclude_graphdriver_btrfs btrfs_noversion exclude_graphdriver_devicemapper containers_image_openpgp" \
-o bin/darwin/podman ./cmd/podman
# github.com/containers/podman/v5/cmd/podman
usage: link [options] main.o
...
make[1]: *** [bin/darwin/podman] Error 1
make: *** [pkginstaller] Error 2
@@ -119,6 +119,7 @@ LDFLAGS_PODMAN ?= \ | |||
-X $(LIBPOD)/config._installPrefix=$(PREFIX) \ | |||
-X $(LIBPOD)/config._etcDir=$(ETCDIR) \ | |||
-X $(PROJECT)/v5/pkg/systemd/quadlet._binDir=$(BINDIR) \ | |||
-X $(PROJECT)/v5/pkg/machine/ocipull.DefaultPolicyJSONPath=$(MACHINE_POLICY_JSON_DIR) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like during the pkginstaller build, $(MACHINE_POLICY_JSON_DIR) isn't passed from package.sh for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for checking, yes the quoting in package.sh was incorrect please retry
e40f43f
to
173b0d0
Compare
One last fix:
With this, policy.json is installed into |
Thanks @ashley-cui, I will apply this on the next push. |
I think there is 3 ways this could work:
The advantage of 1 over 2 is that a developer who does "make podman-remote" and then copies over the exe gets the same behavior as the zip build The advantage of 3 over 1 and 2 is that it's the same no matter how the binary is built (including when the winemake powershell build is used). Relatedly, If you go with 1 or 2, then then winmake.ps1 would also need an update as a followup to set this |
173b0d0
to
ba3e3c5
Compare
@n1hility I went with 2, leave it to you to add it to the installer and winmake. I rather not touch more of this right now, |
@Luap99 sure SGTM |
contrib/pkginstaller/Makefile
Outdated
mkdir $(PACKAGE_ROOT)/podman/config | ||
cp ../../pkg/machine/ocipull/policy.json $(PACKAGE_ROOT)/podman/config/policy.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to be moved into the package_root target in order to be correctly packaged.
clean-binaries podman-remote; \ | ||
else \ | ||
$(MAKE) $(GOPLAT) podman-remote; \ | ||
$(MAKE) $(GOPLAT) MACHINE_POLICY_JSON_DIR="." podman-remote; \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is going to apply this setting to all machine zip archives whether darwin/win/linux etc, which is probably not what you want.
One other approach that could make the Makefile approach a bit easier is to move LDFLAGS_PODMAN down to be after the os check area where SRCBINDIR is defined, and use that to also set MACHINE_POLICY_JSON_DIR defaults.
Or you could this code wise and just change the logic in policyPath with a func in _windows.go that ignores the build flag and just uses a hard coded constant.
If you prefer I can also just address this aspect in the follow-up PR change that also updates the installer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this is going to apply this setting to all machine zip archives whether darwin/win/linux etc, which is probably not what you want.
Why not? If you run from the zip we should not assume any hard coded locations so I think that is fine regardless of the OS that they are used on.
I rather not hard code the default into the binary only for windows as this should really left to packagers IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok fair enough, I assumed you wanted something like ../../conf on linux but I agree no reason it can't all be the same.
There is one remaining issue which is that right now the policy is placed in the root, yet the binary is placed in usr/bin. For this strategy to work with the zips we would need to put the policy in /usr/bin, or alternatively we could do a search order, first check same dir, then fall back to ../..
@Luap99 @n1hility @ashley-cui This ready to merge? |
From my side yes, it will need follow up changes for the windows installer but this is way beyond me so I cannot do it in this PR. This PR adds the build option and fixes our mac installer so it should already be a good improvement. |
I had one unaddressed comment, some lines needed to be moved for proper mac installer fixes. |
LGTM just one remaining small issue (See thread above), but I'm fine with us dealing with it in a follow up PR |
A sorry looks I screwed that up while rebasing, let me fix it real quick. |
The default policy file /etc/containers/policy.json location does not work on windows and for packages that ship a default. Now we search for the policy.json in the following overwrite locations: macos and linux: - ~/.config/containers/policy.json - /etc/containers/policy.json windows: - %APPDATA%\containers\policy.json Also it offers an additional DefaultPolicyJSONPath var that should be overwritten at built time with the path of the file that is shipped by packagers. Thile file is used when none of the overwrite paths exist. [NO NEW TESTS NEEDED] Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Allow users to set MACHINE_POLICY_JSON_DIR to the policy.json directory which is used for podman machine pulls. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
So that this file can be inculded in our windows/macos packages and also by other packagers. Right now the default policy is allow everything but we plan to add signing in the future. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
Include a default policy.json file in the macos package so users do not have to add this manually. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
This config needs to be included for podman machine pulls to work and set MACHINE_POLICY_JSON_DIR so that the file should be located next to the binary. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
ba3e3c5
to
e32c9bb
Compare
LGTM, TY! |
/lgtm |
see commits
Does this PR introduce a user-facing change?