In the rootless mode pkg/homedir.GetConfigHome() should be used instead of /etc

Will do, it'll be in a later PR now.

Why opt?

Because the default in the containerd package is /opt/nri/plugins. As also mentioned under "Notes (please read!)" in the description, I'm open to suggestions.

It looks like there's been similar discussions around CNI, where it was considerd a mistake that it used /opt/cni/; also see containerd/containerd#6600

I see that distros are actually patching containerd to use the correct path (/opt/cni/bin -> /usr/lib/cni); https://sources.debian.org/src/containerd/1.7.24~ds1-10/debian/patches/0006-Add-Debian-specific-CNI-bin-dir-to-ctr-run-command.patch

So perhaps /usr/libexec/docker/nri/ and /usr/lib/docker/nri/ would be more suitable paths for this. And perhaps the containerd PR should be reopened for further discussion.

+1 on /usr/libexec/docker/nri/plugins (or .../nri-plugins)

Cool - I'll do that ... deferred to a later PR now.

This feels slightly odd, as it's using the opts.NRIConfigOpt, which is a wrapper around the underlying config;

type NRIConfigOpt struct {
	// Val is a pointer so that, if no JSON/cmd line options are given, Value() returns defaults.
	// It's exported because the value goes missing somewhere in Cobra if it's not.
	Val *NRIConfig
}

Wondering if we can find some way around that, so that the underlying config is available in the Config.CDI struct.

This version is based on the default address pools option ...

But I'll take a look. (Back to NamedOpt-land I go!)

It should be better now ... and the default paths are deferred to a later PR.

Just considering that ... maybe these should be a []string to allow specifying paths (plural) for discovering plugins. As you (correctly) called out; NRI plugins for (say) CRI / kubernetes may not be applicable to non-kubernetes workloads (so having separate locations for docker and non-docker probably makes sense), there could be cases where they ARE expected to be applicable to both.

In the current situation in this PR, a user can configure these paths to match containerd's paths, in which case they're shared, but wouldn't be able to express "share all the containerd NRI plugins AND look or dockerd-specific plugins in <this location>". If we make these a slice of paths, that's something that could be expressed; i.e., the dockerd-specific locations to be expanding the list of NRI plugins that are applied without that meaning "create a copy of all plugins in both places".

In general, I'm still curious if NRI implements / should implement (or define) conditionals; with CDI, the user is in control whether CDI should be applied (and thus modify the OCI spec); for NRI that's not the case; it's unilaterally modifying the behavior of dockerd (containers) to patch the OCI config. I guess that's mostly by design, as NRI is intended to extend the runtime, and of course it could be left to the plugin to handle these conditions, but wondering if there's something to define / formalise on behavior (which could include "disable all NRI for this container / pod").

maybe these should be a []string to allow specifying paths (plural) for discovering plugins

The NRI adaptation code only accepts a single path ... so we could make our daemon config take a list to be more future-proof, but we'd still have to make it an error to specify more than one.

If we don't do that, and find we need to configure multiple paths in the future - we'd need to add <...>Paths flags, and support both. That wouldn't be a disaster.

So I don't feel strongly either way.

.. NRI is intended to extend the runtime, and of course it could be left to the plugin to handle these conditions

Yes, I think that's the philosophy ... the plugin gets to decide. An NRI plugin that's monitoring for new containers in order to do some sort of setup/teardown on the host would need to see all of them, but wouldn't modify any.

As we don't have a requirement for selective NRI-ing at the moment and adding it in future would be non-breaking, I think we leave it for now?

Hm, right; so it DOES look like there's a dropin-path to allow overrides (looks like that corresponds with the conf.d directory approach), but of course that doesn't allow for the "differentiate between k8s and non-k8s situations";
https://github.com/containerd/nri/blob/c485d5fbddea569dfb1feec9b9e42e8d41b5b3af/pkg/adaptation/adaptation.go#L96-L102

@klihub @samuelkarp - I think you both have been more involved in the design for NRI; any thoughts / recommendations here? We're looking at adding NRI support to Moby for non-kubernetes situations, but trying to find the right balance between using the same / default paths used by containerd, and being able to specify more granular options.

As I understand it - the plugin config path is for files named after the plugin, with a .conf extension ... they can contain yaml/json/whatever, and are provided to plugins started because they live in the plugin directory (not plugins that start separately and use the socket, they're expected to deal with their own config). Those files might configure anything in the plugin, paths for host, attributes to match on, actions to take etc etc - it's entirely up to the plugin.

So, a shared plugin directory with config directories per-NRI runtime would be a way to configure the plugins differently ... but it's up to the plugin implementation whether it supports that. A plugin written for Kubernetes wouldn't necessarily be expecting to run under Docker, so might not be disable-able / modifiable for the different use-case. (Hence using different paths to keep things separate by-default, it'd still be possible to configure a shared path if that was useful.)

The NRI adaptation layer is given a name/version ... but I'm not sure the plugins get to know what's running them. Even if they do, it'd be up to the plugin whether it takes any notice and differentiates between Kubernetes/Docker for example.

Discussed on the moby maintainers call ... we'll stick with single plugin / plugin-config directories for now. In future, if there's a case for multiple directories - can can use a path separator in the single string (or add an option to configure a list of directories).

The NRI adaptation layer is given a name/version ... but I'm not sure the plugins get to know what's running them. Even if they do, it'd be up to the plugin whether it takes any notice and differentiates between Kubernetes/Docker for example.

@robmry The plugins do get told what version of which 'runtime' they are connected to, but since currently we only have NRI integration with CRI-compatible runtimes (and effectively only to the CRI-managed containers within containerd), so just like you said I also suspect none of the existing plugins really care or do check it. Unless they need to examine some runtime-specific metadata/annotation on containers, which then might differ between containerd and CRI-O.

Discussed on the moby maintainers call ... we'll stick with single plugin / plugin-config directories for now. In future, if there's a case for multiple directories - can can use a path separator in the single string (or add an option to configure a list of directories).

@robmry And if it helps, and we can figure out what exactly would be needed, we can also add support in NRI to look for plugins in multiple directories, more granular configuration, etc.

Thanks @klihub ! Yes, we were discussing some of those options, and (especially since NRI is not yet v1.0.0), perhaps now is the right moment to start discussing some.

Some things we discussed (further up), and were considering to open tickets for for further discussion;

• (probably containerd specific); the default /opt/xxxxx location seemed non-conventional; there's prior discussion for the same for CNI, but perhaps those defaults should be changed to /usr/lib / /usr/libexec/ to prevent distros from patching the defaults; NRI: add daemon.json/command line options #51634 (comment)
• with the above in mind, perhaps multiple paths would help as well, as it would allow transitioning to new path, but keeping the old locations to help the transition.
• ☝️ but maybe some handling is needed to prevent the same plugin being found in multiple locations 🤔
• The "this NRI is intended for k8s <--> intended for other purposes (or perhaps "both")" discussion; I understand it's largely "by design" to apply most changes unconditional (as the design (to my understanding) is to extend the runtime); NRI is a powerful (and dangerous, so must have trusted plugins) tool, so yes, wondering if there should be more guidance from the specification around this.
• (I could imagine some opt-in/opt-out options to control this per container / pod, but haven't given that a lot of thought)

One, more technical, issue we found was that WASM support has various hard-coded paths; it looks like this dependency causes a 5MB+ increase in our binary sizes, which is substantial.

Initially I thought it would be possible to pass "enable WASM" support as an option for the constructor (which takes functional options, so injecting seemed like a good fit); https://github.com/containerd/nri/blob/8f3e69e159d135a01e64462da2adfb1681bdbfb6/pkg/adaptation/adaptation.go#L169-L175

However, it looks like, while an interface is defined, that interface is not part of the NRI repository itself, and taken from the external dependencies, which also define various types, which not only means that the interface could change through that dependency, but some of the types could become problematic (as was the case with the containerd v1 -> v2 transition);
https://github.com/containerd/nri/blob/8f3e69e159d135a01e64462da2adfb1681bdbfb6/pkg/api/api_host.pb.go#L88-L93

I don't have a good solution for that yet (I was trying locally if forking the interface to be included in NRI would work, but there's quite some code involved there, including some implicit dependencies)

Thanks @klihub ! Yes, we were discussing some of those options, and (especially since NRI is not yet v1.0.0), perhaps now is the right moment to start discussing some.

Some things we discussed (further up), and were considering to open tickets for for further discussion;

• (probably containerd specific); the default /opt/xxxxx location seemed non-conventional; there's prior discussion for the same for CNI, but perhaps those defaults should be changed to /usr/lib / /usr/libexec/ to prevent distros from patching the defaults; NRI: add daemon.json/command line options #51634 (comment)
• with the above in mind, perhaps multiple paths would help as well, as it would allow transitioning to new path, but keeping the old locations to help the transition.
• ☝️ but maybe some handling is needed to prevent the same plugin being found in multiple locations 🤔

@thaJeztah I think as long as NRI can go ahead and start plugins from all the known locations, this should be doable. If there is a clear set of more reasonable paths towards which we should transition, I think we can do it. And we should be able to filter multiple instances of the same plugin. Either by stat'ing to see if they are the same file (plugins are supposed to be symlinked in the predefined plugin directories), or simply by the instances having identical name and index, in which case we can drop the duplicates during initial discovery.

• The "this NRI is intended for k8s <--> intended for other purposes (or perhaps "both")" discussion; I understand it's largely "by design" to apply most changes unconditional (as the design (to my understanding) is to extend the runtime); NRI is a powerful (and dangerous, so must have trusted plugins) tool, so yes, wondering if there should be more guidance from the specification around this.
• (I could imagine some opt-in/opt-out options to control this per container / pod, but haven't given that a lot of thought)

There has been some initial work done on this front. We've added 'pluggable validation' (and a configurable built-in default validator plugin) to allow locking down some of the features. The (rejection) semantics might not be what you are after though.

One, more technical, issue we found was that WASM support has various hard-coded paths; it looks like this dependency causes a 5MB+ increase in our binary sizes, which is substantial.

Initially I thought it would be possible to pass "enable WASM" support as an option for the constructor (which takes functional options, so injecting seemed like a good fit); https://github.com/containerd/nri/blob/8f3e69e159d135a01e64462da2adfb1681bdbfb6/pkg/adaptation/adaptation.go#L169-L175

However, it looks like, while an interface is defined, that interface is not part of the NRI repository itself, and taken from the external dependencies, which also define various types, which not only means that the interface could change through that dependency, but some of the types could become problematic (as was the case with the containerd v1 -> v2 transition); https://github.com/containerd/nri/blob/8f3e69e159d135a01e64462da2adfb1681bdbfb6/pkg/api/api_host.pb.go#L88-L93

I don't have a good solution for that yet (I was trying locally if forking the interface to be included in NRI would work, but there's quite some code involved there, including some implicit dependencies)

Maybe the easiest/first step to help with the 5MB+ binary size hit would be to see if we can put the WASM-related bits behind a build flag, so that it becomes possible to swap them out for nop/error stubs.

Last bike-shed for this PR; wondering if a slightly more verbose, but perhaps more descriptive nri-opts or nri-config would make sense. I see we use opts for various other ones, so could perhaps work, but off course our type is named NRIConfig, so 🤷

(Happy to hear your thoughts though!)

Done - it's now NRIOpts in the Go, "nri-opts" in the JSON, and --nri-opts on the command line.