Quadlet container mount - support non key=val options

[ygalblum]

Some keys, e.g. ro do not have values.
The current implementation crashed looking for the = sign

Externalize findMountType in a new package
Parse mount command using FindMountType
Add test case and adjust the test framework

Does this PR introduce a user-facing change?

Yes

Quadlet container mount - support non key=val options

Resolves: #20104

[ygalblum]

Sorry for opening another PR for this issue while #20108 and #20112 are open. I wasn't sure if I can push to these branches.
This change addresses the example issue in #20104 and as raised by @Luap99 here

[ygalblum]

@vrothberg @rhatdan @Luap99 @giuseppe PTAL

[vrothberg]

I think that the underlying problem of having two separate functions to parse essentially the same thing remains. That implies a risk of code divergence and subtle errors.

Did you consider recycling the Podman-internal function to parse mounts and change it in a way that we can re-use it in Quadlet?

[ygalblum]

I think that the underlying problem of having two separate functions to parse essentially the same thing remains. That implies a risk of code divergence and subtle errors.

Did you consider recycling the Podman-internal function to parse mounts and change it in a way that we can re-use it in Quadlet?

My idea was to limit the parsing to only what Quadlet really needs and that is resolving the .volume case. Unlike the original implementation all the rest is not looked or changed. As such the assumptions are only that:

1. The parameter separator it ,
2. type, source and src are in the key=value format

I think that reusing the Podman parser code might raise other issues. For example, Quadlet is not supposed to do any validity checks. But, now, it will.

[Luap99]

I think that the underlying problem of having two separate functions to parse essentially the same thing remains. That implies a risk of code divergence and subtle errors.

Did you consider recycling the Podman-internal function to parse mounts and change it in a way that we can re-use it in Quadlet?

Exactly, --mount flag uses csv not split at comma, this behaves differently for special chars so it is important to use the same code. And for the future someone might change the --mount parsing and then will not update quadlet so it will diverge again. The only way to address this is to directly share the code.

[vrothberg]

I think that reusing the Podman parser code might raise other issues. For example, Quadlet is not supposed to do any validity checks. But, now, it will.

That's a fair argument. I think we mostly avoid validation to avoid code duplication/divergence. But if we manage to share the same function we could do it.

But I guess it would at some dependencies to Quadlet (likely runtime spec).

[Luap99]

you can just use

podman/pkg/specgenutil/volumes.go

Line 145 in a2434a9

func findMountType(input string) (mountType string, tokens []string, err error) {

That should already work for both use cases podman and quadlet. Simply move this function to a new package without all the extra imports as that would bloat quadlet otherwise.

[ygalblum]

@vrothberg @Luap99 Done. PTAL

[Luap99]

This will still fail when the value contains special chars, you must again encode this via the csv lib in order to encode this back into the right format. Consider a a filepath with comma:
--mount=type=bind,src=/tmp,"dst=/path,1"

[vrothberg]

Great catch. Once fixed, can we also add a test case for it?

[ygalblum]

Adding the test case is the first step in writing the fix :)

[vrothberg]

Like Batman, you are not the maintainer we deserve but the one we need :^)

[ygalblum]

Fixed. Wow, making the test work was more complicated than the code fix :)

[vrothberg]

LGTM

[ygalblum]

Does this failure block the merge: bin/quadlet grew by 53951 bytes; max allowed is 51200.?

[Luap99]

Does this failure block the merge: bin/quadlet grew by 53951 bytes; max allowed is 51200.?

Yes but we can overwrite with the label, given this is so close I am good with it. You already split out the function to not require unnecessary imports so I assume this is the best we can do and the grow is mostly caused by the CSV parser.

[TomSweeneyRedHat]

LGTM

[vrothberg]

LGTM, thanks @ygalblum !
I let @Luap99 have a final head nod

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg, 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 [vrothberg,ygalblum]

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

[Luap99]

/lgtm

Thanks