pkg/hooks: Version the hook structure and add 1.0.0 hooks

[wking]

This shifts the matching logic out of libpod/container_internal and into the hook package, where we can reuse it after vendoring into CRI-O (as @rhatdan suggested here). It also adds unit tests with almost-complete coverage. Now libpod is even more isolated from the hook internals, which makes it fairly straightforward to bump the hook config file to 1.0.0. I've dubbed the old format 0.1.0, although it doesn't specify an explicit version. Motivation for some of my changes with 1.0.0:

• Add an explicit version field. This will make any future JSON structure migrations more straightforward by avoiding the need for version-guessing heuristics.

• Collect the matching properties in a new When sub-structure. This makes the root Hook structure easier to understand, because you don't have to read over all the matching properties when wrapping your head around Hook.

• Replace the old hook and arguments with a direct embedding of the runtime-spec's hook structure. This provides access to additional upstream properties (args[0], env, and timeout) and avoids the complication of a libpod-specific analog structure.

• Add a when.always property. You can usually accomplish this effect in another way (e.g. when.commands = [".*"]), but having a boolean explicitly for this use-case makes for easier reading and writing.

• Replace the previous annotations array with an annotations map. The 0.1.0 approach matched only the values regardless of key, and that seems unreliable.

• Replace cmds with when.commands, because while there are a few ways to abbreviate “commands”, there's only one way to write it out in full ;). This gives folks one less thing to remember when writing hook JSON.

I've also added doc-compat support for the various pluralizations of the 0.1.0 properties. Previously, the docs and code were not in agreement (e.g. doc stages vs. code stage). More on this particular facet in cri-o/cri-o#1235.

And I've updated the docs to point out that the annotations being matched are the OCI config annotations. This differs from CRI-O, where the annotations used are the Kubernetes-supplied annotations. For example, io.kubernetes.cri-o.Volumes is part of CRI-O's runtime config annotations, but not part of the Kubernetes-supplied annotations CRI-O uses for matching hooks.

The Monitor method supports the CRI-O use-case. podman doesn't need it directly, but CRI-O will need it when we vendor this package there.

The WIP in the subject is because I'm not sure what the has-bind-mounts logic should look like, although I think the current len(c.config.Spec.Mounts) > 0 is not quite what we want.

[rh-atomic-bot]

Can one of the admins verify this patch?
I understand the following commands:

• bot, add author to whitelist
• bot, test pull request
• bot, test pull request once

[mheon]

bot, add author to whitelist

[rhatdan]

Shouldn't this be a return err. Panic would bring cri-o down.

[mheon]

Agree, should probably have an ErrUnknownStage and return errors.Wrapf(ErrUnknownStage, "invalid stage %q", stage) or similar

[wking]

Shouldn't this be a return err. Panic would bring cri-o down.

This should be impossible, because we validate stages after reading them off the disk. The Manager hook information is private, so I'm not aware of a way to get around the post-read validation and inject a buggy hook. So I don't think the panic is an issue, but I'll push an update anyway. There could always be bugs I've missed ;).

[wking]

Agree, should probably have an ErrUnknownStage and return errors.Wrapf(ErrUnknownStage, "invalid stage %q", stage) or similar

Somehow I missed this comment earlier. I've updated this line since to return an error, but have not defined an ErrUnknownStage. Do you expect callers to want to switch on that vs. other error conditions? I'm having trouble imagining when you'd want different handling than for, say, invalid-regexp errors.

[mheon]

If we don't see this being hit often at all (and given your earlier comment on validation elsewhere, I'm content to believe that), there's no real point to having a specific error -- if we didn't have such validation, I think it would be helpful to have an identifier for a potentially common error

[rhatdan]

Do existing hooks have a version? I don't think so, we would need to warn and set the version to 1.0.

[wking]

Do existing hooks have a version? I don't think so, we would need to warn and set the version to 1.0.

I'm retroactively declaring them to be 0.1.0, see the initial comment in this PR, these docs, and this code.

[rhatdan]

@baude @mheon PTAL

[baude]

LGTM, needs gofmt ... remember to sign all the commits ...

[baude]

also agree on the error handling

[mheon]

I think this can probably be len(c.config.Spec.Mounts) > 0... It's not 1:1 with actual bind mounts, but it's the closest we can get inside libpod

[wking]

I think this can probably be len(c.config.Spec.Mounts) > 0... It's not 1:1 with actual bind mounts, but it's the closest we can get inside libpod

We can always adjust the libpod API to be more precise. However, you're suggesting we just keep the current logic, so I expect we should punt any libpod API changes to follow-up work if we address this issue at all.

Can you recommend wording for the the docs explaining the difference, so we document the behavior to avoid surprising users?

[mheon]

On further examination, we do add several mounts unconditionally to the spec by default, so my change compiles into an unconditional true, avoiding the actual intent of the hook...

Given this, I think it's safest right now to keep your unconditional false and document that hasBindMounts hooks are not currently implemented. We could add a boolean to config indicating that user-added bind mounts are present for the purpose of informing the hooks at a later date.

[wking]

Given this, I think it's safest right now to keep your unconditional false and document that hasBindMounts hooks are not currently implemented.

Done and pushed as 51fb6c48.

[rhatdan]

oci-umount relies on this. So it would be better if you unconditionally return true, until it is fixed. I would rather always run the hook and take the penalty rather then face problems with leaked mount points. But we need to fix this quickly.

[wking]

But we need to fix this quickly.

It's probably better if @mheon or someone else familiar with the config flow files a separate PR to update libpod to handle this correctly. I'll rebase this on top once that lands.

[mheon]

@wking Sure, should be simple enough. I'll see about getting one in on Monday.

[wking]

Should it be an error to call podman without the configured hook directory? Currently the tests fail with:

Running: /usr/bin/podman --root /tmp/podman_test502773819/crio --runroot /tmp/podman_test502773819/crio-run --runtime /usr/bin/runc --conmon /usr/libexec/crio/conmon --cni-config-dir /etc/cni/net.d --storage-driver=vfs run -it -v /tmp/podman_test502773819/socket:/h registry.fedoraproject.org/fedora-minimal:latest bash -c trap "echo FOO >> /h/fifo " 8; echo READY >> /h/fifo; while :; do sleep 0.25; done
error setting up OCI Hooks: open /usr/share/containers/oci/hooks.d: no such file or directory

I can use IsNotExist to make that particular error (vs., for example, an EPERM) non-fatal. But is that the expected interface? If called podman with an explicit --hooks-dir-path=/some/typoed/directory, I'd expect an error.

Should we keep the error fatal in libpod, remove podman's default, and just skip hooks entirely unless --hooks-dir-path is explicitly set? I think that would be the cleanest long-term approach, although we'd need a migration period to shift existing implicit-hooks users over to the new approach. We could handle that in podman though, and immediately transition libpod over to requiring the hook directory passed in via WithHooksDir be readable by the time we need the hooks.

[mheon]

@wking I think that if the default hooks directory does not exist, it should not be fatal. But if a directory was explicitly set and does not exist, that should be an error. I'd add a check to make sure the path we give to WithHooksDir() in options.go exists, and make a nonexistant directory within the actual hooks package nonfatal.

[wking]

I think that if the default hooks directory does not exist, it should not be fatal. But if a directory was explicitly set and does not exist, that should be an error.

Ok, so:

• “error if the caller explicitly provided an invalid directory”. I'm on board with that.
• “do not error if someone calls podman without specifying a directory but no hook directory exists”. I'm on board with that.

Do you have a position on “migrate away from the current default hook directory and require callers who need hooks to set the default directory explicitly”? I'm in weakly favor of that, but for now I'll assume you want to keep the existing default.

I'd add a check to make sure the path we give to WithHooksDir() in options.go exists…

Do we need to bother there? A check at that point would be racy vs. checking at invocation time. If we want “directory was explicitly set and does not exist” to be fatal (and we seem to agree that we do), then I'd rather enforce that check in the hooks package (as I do in this PR), and make the failed-race behavior too strict instead of making it too permissive.

… and make a nonexistant directory within the actual hooks package nonfatal.

I think we want to keep this fatal because of the above race. I've pushed acc35edc0 with one potential implementation for the above criteria that makes the failed-race behavior too strict.

[mheon]

@wking I like enforcing restrictions in WithHooksDir() (and the other With...() functions) because they're the logical border of libpod and Podman, and act as part of libpod's public API. As such, when we move over to CRI-O (hopefully beginning soon, I'm finishing some of the last work on that front), we'll retain the benefits of the validation there.

I think the checks you added seem reasonable, though they should probably be moved into WithHooksDir(). This way, we validate the hooks directory once as it is passed into libpod and again when we actually use it, avoiding races (and incurring overhead, but I'm OK with an extra stat call as we start up). I would like to see a check in the hooks themselves to make the directory not existing not fatal only if the directory is the default contained in the hooks package (so we can guarantee that, on an unconfigured system with no default directory, we can still run Podman without errors).

[wking]

I think the checks you added seem reasonable, though they should probably be moved into WithHooksDir().

I've kicked this around a bit, but can't get an API I'm comfortable with. We can't just have WithHooksDir return a RuntimeOption that returns an error when the directory is missing (like I'm doing here), because NewRuntime doesn't provide a way to continue subsequent processing after a RuntimeOption returns an error. So any error returned from WithHooksDir's RuntimeOption has to be fatal.

You could do something like:

func WithHooksDir(hooksDir string, dirNotExistFatal bool) RuntimeOption

to make the returned error conditional, but then there's no way for the caller to know if the directory was successfully added or not (e.g. if it wanted to pass a warning up the call chain).

Are we really settled on the NewRuntime(options) approach? If we shifted the With… setters into runtime methods:

runtime, err := libpod.NewRuntime()
…

err = runtime.SetHooksDir(c.GlobalString("hooks-dir-path"))
if err != nil && (c.GlobalIsSet("hooks-dir-path") || !os.IsNotExist(err)) {
  return nil, err
}

err = runtime.Finalize()
return runtime, err

callers would have much more flexibility about handling setter errors.

This way, we validate the hooks directory once as it is passed into libpod and again when we actually use it, avoiding races…

You'd still have the race I have in acc35edc0f, where the directory is stat-able at runtime-creation-time but no longer in existence by the time we actually try and load the hooks, with callers who did not set --hooks-dir-path getting the does-not-exist there when they shouldn't. That's the “too strict” behavior, and I'm ok with it. If you wanted to avoid the race, you'd have to pass down a “considers non-existant-hook-dir fatal” boolean or similar into libpod, while we're currently just passing in a hook-dir path.

On the other hand, the WithHooksDir(hooksDir string, dirNotExistFatal bool) signature I mention above would be one way to get the information you'd need to pass down, and it could set both rt.config.HooksDir (like it already does) and a new rt.config.DirNotExistFatal. Then we could text in WithHooksDir or not, as you like, and then test again with the appropriate fatal-ness at hook-load time.

I would like to see a check in the hooks themselves to make the directory not existing not fatal only if the directory is the default contained in the hooks package (so we can guarantee that, on an unconfigured system with no default directory, we can still run Podman without errors).

I'm currently very happy with pkg/hooks not consuming its directory constants at all. Things like CRI-O's “if the user set a non-default hook dir, drop the usual override directory” seem needlessly brittle to bake into the hooks library itself. By not baking in a DefaultDir condition, we avoid issues for folks who want to support, say, unprivileged users who read hooks from a per-user directory. Or for callers who have different opinions about where hooks should go by default. If you really need to avoid the race, then I'd much prefer passing a HooksNewErrorFatal or similar config down through libpod, which you could consume like:

manager, err := hooks.New(ctx, []string{c.runtime.config.HooksDir})
if err != nil {
  if c.runtime.config.HooksNewErrorFatal {  // or something that only softens the directory-does-not-exist case
    return err
  }
  logrus.Warn(err)
}

[mheon]

I'm fine with the error from WithHooksDir being fatal - explicitly setting hooks to a directory that doesn't exist sounds like an error. If we actually want to clear them, a separate function (WithNoHooks() or similar) sounds more reasonable to me than accepting "" as a way to clear the hooks.

Your second suggestion, passing down an error, sounds fine, though I think we'd be better off just passing through an error from os.Stat() so we can use the built-in os.IsNotExist() function to check if we actually had a fatal init error, or the directory we're using as hooks doesn't exist (probably just a logged warning)

[wking]

If we actually want to clear them, a separate function (WithNoHooks() or similar) sounds more reasonable to me than accepting "" as a way to clear the hooks.

WithHooks(hooksDir *string, …) with nil to clear would be explicit enough for me if we wanted to have empty-string values mean the current directory. I'm also fine documenting empty-string values as clearing if we want. A WithNoHooks() is nice and explicit, but then callers have to switch between the two functions and switch based on their own *string or bool/string pair. Anyhow, for now I've pushed 9394b2f1 to have WithHooks error on empty strings, and we can sort out the clearing API later.

Your second suggestion, passing down an error, sounds fine, though I think we'd be better off just passing through an error from os.Stat() so we can use the built-in os.IsNotExist() function to check if we actually had a fatal init error, or the directory we're using as hooks doesn't exist (probably just a logged warning)

A Stat call would be racy. When hooks.New is called with a non-existent directory, it's the ioutil.ReadDir that would return the not-exist error, since that's the only operation we need to perform on the directory. 9394b2f1 adds a new HooksDirNotExistFatal libpod config setting and sets it via:

func WithHooksDir(hooksDir string, dirNotExistFatal bool) RuntimeOption

That allows us to avoid the os.Stat/ioutil.ReadDir race by pushing the warn/error switch down into libpod. Does that look good to you?

[mheon]

@wking That implementation looks like a good compromise. We preserve checking at the public bounds of Libpod, and we have a way of ensuring that a non-existant hooks dir is nonfatal.

I'll do one more look over on monday, but I think my concerns are addressed now. Thanks @wking

[rh-atomic-bot]

☀️ Test successful - status-papr
Approved by: mheon
Pushing c8208a8 to master...