Quadlet: Add support for .build files

[jmaibaum]

Does this PR introduce a user-facing change?

Quadlet: Add support for .build files

This adds support for .build Quadlet units, as per discussion in #17043 (reply in thread), basically by mapping Key/Value pairs inside a .build file to podman build command line arguments.

You depend on a container image built by a .build file, by adding Image=name.build to your .container file, similar to how you can depend on an .image file.

Mandatory keys in a .build file under the [Build] group are so far:

[Build]
# Tag the image you intend to build so that container service files can use this name in their command lines 
ImageTag=localhost/imagename
# Either of
File=/path/to/a/Containerfile
# or
SetWorkingDirectory=unit
# to add a build context to `podman build`

Not every single podman build command line argument is being handled, yet. There might be more which would be nice to have before merging (please let me know if you think I missed an important one!), but since adding a new Key/Value pair is a rather cumbersome and boring process, I decided to stop after adding the following:

• all KeyXYZ I found already defined, and which I also found in podman builds supported arguments (notable exceptions from this rule are those I did never use before myself, or those where I found more complicated parsing code in the current quadlet code base)
• a few mandatory things which were missing so far, like KeyFile (podman build's --file, i.e. the Containerfile path), or KeyTarget

I have added e2e tests, and documentation for all currently supported keys/args, closely following what I found for the already existing Quadlet file types.

This is my first contribution to a Go project, so please let me know in case I did not follow best practice or used unidiomatic constructs. I used go fmt on all Go files I touched (except for quadlet_test.go, as I only added single lines here, and wasn't sure if go fmt would alter parts of the file I did not touch), and generally tried to follow naming conventions I found in surrounding code.

I have tested this with a few toy projects locally, and there it did it's job. Apart from adding more command line args if deemed necessary, I consider the implementation ready functionality wise.

Happy to hear what the maintainers think about this!

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Cockpit tests failed for commit e53db304fc55545a388784e19b218f3dc19bc9e2. @martinpitt, @jelly, @mvollmer please check.

[dustymabe]

As the person who started the discussion in #17043 I'd like to say thank you for working on this!

[ygalblum]

Thanks for working on this. Great job.

The dependency comment is obviously the tricky one

[ygalblum]

BTW the build is failing on trailing white spaces in the doc file

[jmaibaum]

BTW the build is failing on trailing white spaces in the doc file

I have fixed this in my latest push.

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[packit-as-a-service]

Cockpit tests failed for commit 9011d457bb9bcb6338fc07cd189a863895fd92b8. @martinpitt, @jelly, @mvollmer please check.

[ygalblum]

Just these two small comments

[packit-as-a-service]

Ephemeral COPR build failed. @containers/packit-build please check.

[ygalblum]

I'm not sure if you have more tests you want to add. Please let me know once you're done. In addition, once you're done, please squash all commits into one

[jmaibaum]

Yes, I am still working on the tests, currently struggling with the ".volume requires .build/.image" tests. I am going to push 2 failing tests shortly, where I do not understand why they fail, as they work correctly when I am running quadlet on these files manually.

And yeah, once I am done I am going to squash all commits into one.

[jmaibaum]

OK, I found the issue with the 2 testcases image.quadlet.volume and build.quadlet.volume. The reason is that handleImageSource() really requires the image names to be found in the resourceNames array. For all other *.quadlet.* tests requiring other quadlet files the handleXYZ() functions fall back to the "default name", like systemd-$name for networks or volumes (and therefore can be tested in isolation without the referenced files to be present).

The two tests succeed when I patch quadlet_test.go to always copy basic.image and basic.build alongside the file to be tested.

Since handleImageSource needs the *.image or *.build files to read the image name from though, I guess we need a special test case type where you can specify additionally required files. Unless you see an easier way, I am going to write this test type function next.

My assumption is that this complication with handleImageSource() was the reason why the

[Volume]
Driver=image
Image=some.image

hasn't been tested before...

[jmaibaum]

After reading the changelog for Podman 5.1.0-rc1 I also added the --group-add support to Build units.

Unless you have further remarks or suggestions @ygalblum , I consider the PR ready. Please let me know and then I will squash the commits into one.

[rhatdan]

Please fix the man pages so the tests will run.

[jmaibaum]

The failing RPM builds seem to be due to infra issues unrelated to the changes in this PR IIUC?

[ygalblum]

Thanks for all your effort.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmaibaum, ygalblum

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 [ygalblum]

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

[jmaibaum]

@ygalblum I have a local WIP commit for supporting URLs, and also for handling more corner cases in terms of relative or absolute file paths to Containerfiles (in both File= and SetWorkingDirectory=). It is not yet ready (still fighting with a few of those corner cases), but I think it makes sense to wait with merging until these useful features (as mentioned in the code conversation above) can be handled correctly.

IIUC, this PR would be targeting a release with Podman 5.2.0 earliest, as the 5.1.0-rc period has already started. So, I would like to take more time to polish this.

[jmaibaum]

@ygalblum Initial support for URLs pushed. I did this in a separate commit on purpose to be able to revisit it again in case you (or another maintainer) don't like it. I will squash again if this is acceptable.

Also, I would be really glad if someone could give this a test spin. As noted above, I had not been using the URL features of podman build before myself. More eyes (and test runs) welcome!

[ygalblum]

Looks good. Though I haven't tested it.
Maybe you should consider adding a system test as well (in test/system/252-quadlet.bats)

[jmaibaum]

I have to see if I can get the system tests to run on my machine. I was struggling quite a lot to get the e2e tests running on my machine at least. (For example, I only get them to run in rootful mode. I never managed to get rootless e2e working locally. Might be related that I am using toolbox on Fedora Silverblue, so it's effectively podman in podman. Never did that before so I don't know which screw I might have forgotten to tighten here.)

[jmaibaum]

I tried to get the system tests working locally, but failed both for --rootless and --root, most likely because I am running them inside https://containertoolbx.org/ on Fedora Silverblue 40 (i.e. it's podman inside podman).

The --rootless tests fail due to some issue with uid maps:

⬢[jm@toolbox ~]$ ./hack/bats --rootless 252:"quadlet - basic"
--------------------------------------------------
$ bats --filter quadlet - basic test/system/252-quadlet.bats
252-quadlet.bats
 ✗ quadlet - basic
   [teardown] $ podman pod rm -t 0 --all --force --ignore
   time="2024-05-27T16:25:54+02:00" level=error msg="running `/usr/bin/newuidmap 7575 0 1000 1 1 100000 65536`: newuidmap: write to uid_map failed: Operation not permitted\n" time="2024-05-27T16:25:54+02:00" level=error msg="invalid internal status, try resetting the pause process with \"/var/home/jm/go/src/github.com/containers/podman/bin/podman system migrate\": cannot set up namespace using \"/usr/bin/newuidmap\": exit status 1"
   [teardown] $ podman rm -t 0 --all --force --ignore
   time="2024-05-27T16:25:54+02:00" level=error msg="running `/usr/bin/newuidmap 7593 0 1000 1 1 100000 65536`: newuidmap: write to uid_map failed: Operation not permitted\n" time="2024-05-27T16:25:54+02:00" level=error msg="invalid internal status, try resetting the pause process with \"/var/home/jm/go/src/github.com/containers/podman/bin/podman system migrate\": cannot set up namespace using \"/usr/bin/newuidmap\": exit status 1"
   [teardown] $ podman network prune --force
   time="2024-05-27T16:25:54+02:00" level=error msg="running `/usr/bin/newuidmap 7611 0 1000 1 1 100000 65536`: newuidmap: write to uid_map failed: Operation not permitted\n" time="2024-05-27T16:25:54+02:00" level=error msg="invalid internal status, try resetting the pause process with \"/var/home/jm/go/src/github.com/containers/podman/bin/podman system migrate\": cannot set up namespace using \"/usr/bin/newuidmap\": exit status 1"
   [teardown] $ podman volume rm -a -f
   time="2024-05-27T16:25:54+02:00" level=error msg="running `/usr/bin/newuidmap 7630 0 1000 1 1 100000 65536`: newuidmap: write to uid_map failed: Operation not permitted\n" time="2024-05-27T16:25:54+02:00" level=error msg="invalid internal status, try resetting the pause process with \"/var/home/jm/go/src/github.com/containers/podman/bin/podman system migrate\": cannot set up namespace using \"/usr/bin/newuidmap\": exit status 1"
   (from function `bail-now' in file test/system/helpers.bash, line 227,
    from function `die' in file test/system/helpers.bash, line 813,
    from function `run_podman' in file test/system/helpers.bash, line 423,
    from function `basic_setup' in file test/system/helpers.bash, line 142,
    from function `setup' in test file test/system/252-quadlet.bats, line 27)
     `basic_setup' failed
   
   [16:25:54.037398006] $ /var/home/jm/go/src/github.com/containers/podman/bin/podman rm -t 0 --all --force --ignore
   [16:25:54.175595621] time="2024-05-27T16:25:54+02:00" level=error msg="running `/usr/bin/newuidmap 7534 0 1000 1 1 100000 65536`: newuidmap: write to uid_map failed: Operation not permitted\n"
   time="2024-05-27T16:25:54+02:00" level=error msg="invalid internal status, try resetting the pause process with \"/var/home/jm/go/src/github.com/containers/podman/bin/podman system migrate\": cannot set up namespace using \"/usr/bin/newuidmap\": exit status 1"

Yet, AFAICT I have set up UID mapping correctly:

⬢[jm@toolbox ~]$ getcap $(which newuidmap)
/usr/bin/newuidmap cap_setuid=ep
⬢[jm@toolbox ~]$ cat /etc/subuid
jm:100000:65536

The --root tests fail due to file permissions:

⬢[jm@toolbox ~]$ sudo ./hack/bats --root 252:"quadlet - basic"
# bats --filter quadlet - basic test/system/252-quadlet.bats
252-quadlet.bats
 ✗ [252] quadlet - basic
   (from function `bail-now' in file test/system/helpers.bash, line 235,
    from function `assert' in file test/system/helpers.bash, line 929,
    from function `run_quadlet' in file test/system/252-quadlet.bats, line 64,
    in test file test/system/252-quadlet.bats, line 164)
     `run_quadlet "$quadlet_file"' failed
   
   [16:26:38.037388720] # /var/home/jm/go/src/github.com/containers/podman/bin/podman rm -t 0 --all --force --ignore
   
   [16:26:38.182784656] # /var/home/jm/go/src/github.com/containers/podman/bin/podman ps --all --external --format {{.ID}} {{.Names}}
   
   [16:26:38.311321046] # /var/home/jm/go/src/github.com/containers/podman/bin/podman images --all --format {{.Repository}}:{{.Tag}} {{.ID}}
   [16:26:38.427485648] quay.io/libpod/testimage:20240123 1f6acd4c4a1d
   # /var/home/jm/go/src/github.com/containers/podman/bin/quadlet  /run/systemd/system
   quadlet-generator[8061]: generating service file /run/systemd/system/basic_by6ktASTep.service: open /run/systemd/system/basic_by6ktASTep.service: permission denied

So, I'm afraid I can't add system tests, because I cannot meaningfully test them locally. If they are required before this PR can be merged, I will need help fixing any of the above errors.

[ygalblum]

@jmaibaum I think we can do with the system tests for now. I've restarted the failed tests (which seem unrelated to this change)

[ygalblum]

@containers/podman-maintainers PTAL

[rhatdan]

/lgtm
Great work @jmaibaum